From 962b016cbd24afd65cbc8b7d802ccf21dd93d271 Mon Sep 17 00:00:00 2001 From: Serhii Dziupin Date: Fri, 14 Aug 2026 18:52:48 +0000 Subject: [PATCH 1/5] fix(github): stop stale merged PRs from sticking in branch status Branch PR status now resolves open PRs only, revalidates closed/merged associations on a discovery cadence, and clears authoritative empty results so the panel can self-heal without a manual refresh. Co-authored-by: Serhii Dziupin --- .../views/git/PullRequestSection.tsx | 14 +- packages/ui/src/stores/DOCUMENTATION.md | 4 + .../src/stores/useGitHubPrStatusStore.test.ts | 245 +++++++++++++++++- .../ui/src/stores/useGitHubPrStatusStore.ts | 62 ++++- .../web/server/lib/github/DOCUMENTATION.md | 12 +- packages/web/server/lib/github/pr-status.js | 135 +++++----- .../web/server/lib/github/pr-status.test.js | 89 +++++++ 7 files changed, 470 insertions(+), 91 deletions(-) create mode 100644 packages/web/server/lib/github/pr-status.test.js diff --git a/packages/ui/src/components/views/git/PullRequestSection.tsx b/packages/ui/src/components/views/git/PullRequestSection.tsx index 96eeeee3..19a299c0 100644 --- a/packages/ui/src/components/views/git/PullRequestSection.tsx +++ b/packages/ui/src/components/views/git/PullRequestSection.tsx @@ -1167,21 +1167,19 @@ export const PullRequestSection: React.FC<{ }, [remotes, status?.resolvedRemoteName]); React.useEffect(() => { - const isTerminal = status?.pr?.state === 'closed' || status?.pr?.state === 'merged'; + // Terminal (closed/merged) status must still revalidate on focus/visibility: + // the branch may now have a newer open PR, or the association may clear. const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; const isStale = Date.now() - lastRefreshAt > 60_000; - const shouldRefresh = !isTerminal && isStale; const onFocus = () => { - if (shouldRefresh) { + if (isStale) { void refresh({ force: true, silent: true }); } }; const onVisibility = () => { - if (document.visibilityState === 'visible') { - if (shouldRefresh) { - void refresh({ force: true, silent: true }); - } + if (document.visibilityState === 'visible' && isStale) { + void refresh({ force: true, silent: true }); } }; @@ -1191,7 +1189,7 @@ export const PullRequestSection: React.FC<{ window.removeEventListener('focus', onFocus); document.removeEventListener('visibilitychange', onVisibility); }; - }, [refresh, status?.pr?.state, statusEntry?.lastRefreshAt]); + }, [refresh, statusEntry?.lastRefreshAt]); React.useEffect(() => { if (githubAuthChecked && githubAuthStatus?.connected === false) { diff --git a/packages/ui/src/stores/DOCUMENTATION.md b/packages/ui/src/stores/DOCUMENTATION.md index 4f17d6e4..ee2295e0 100644 --- a/packages/ui/src/stores/DOCUMENTATION.md +++ b/packages/ui/src/stores/DOCUMENTATION.md @@ -173,6 +173,10 @@ Important properties: - `refreshTargets()` supports one-shot multi-target bootstrap without turning on live watching - runtime reset disposes timers, watchers, API references, and request ownership while inert namespaced snapshots remain isolated - persisted cache is versioned, TTL-filtered, and bounded for page refresh continuity, not broad background syncing +- closed/merged associations use the same `5m` discovery cadence as missing PRs so a newer open PR (or authoritative `pr: null`) can replace them without a manual refresh +- closed/merged branch associations are not persisted; legacy hydrated terminal PRs are stripped to `pr: null` and marked unresolved until refresh +- sibling remote-key seeding never copies a closed/merged association +- a successful refresh that returns `pr: null` replaces any previously cached PR authoritatively ## Ownership Rules diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts index b67a5493..61ee2198 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, mock, test } from "bun:test" +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test" import type { GitHubPullRequestStatus, RuntimeAPIs } from "@/lib/api/types" let runtimeKey = "runtime-a" @@ -166,3 +166,246 @@ describe("GitHub PR status cache ownership", () => { expect(useGitHubPrStatusStore.getState().entries[key]?.isLoading).toBe(false) }) }) + +describe("GitHub PR status stale terminal associations", () => { + const originalSetInterval = globalThis.setInterval + const originalSetTimeout = globalThis.setTimeout + const originalClearInterval = globalThis.clearInterval + const originalClearTimeout = globalThis.clearTimeout + let intervalCallbacks: Array<() => void> = [] + + beforeEach(() => { + runtimeKey = "runtime-a" + intervalCallbacks = [] + + const setIntervalStub = ((handler: TimerHandler) => { + if (typeof handler === "function") { + intervalCallbacks.push(handler as () => void) + } + return 1 + }) as unknown as typeof setInterval + const setTimeoutStub = (() => 1) as unknown as typeof setTimeout + const clearIntervalStub = (() => undefined) as typeof clearInterval + const clearTimeoutStub = (() => undefined) as typeof clearTimeout + + globalThis.setInterval = setIntervalStub + globalThis.setTimeout = setTimeoutStub + globalThis.clearInterval = clearIntervalStub + globalThis.clearTimeout = clearTimeoutStub + + // bun:test has no DOM; the store uses window timers and optional document visibility. + Object.assign(globalThis, { + window: { + setInterval: setIntervalStub, + setTimeout: setTimeoutStub, + clearInterval: clearIntervalStub, + clearTimeout: clearTimeoutStub, + }, + document: { visibilityState: "visible" }, + }) + + useGitHubPrStatusStore.setState({ entries: {}, activeRequestCount: 0, totalRequestCount: 0 }) + useGitHubPrStatusStore.getState().resetForRuntimeSwitch() + }) + + afterEach(() => { + useGitHubPrStatusStore.getState().resetForRuntimeSwitch() + globalThis.setInterval = originalSetInterval + globalThis.setTimeout = originalSetTimeout + globalThis.clearInterval = originalClearInterval + globalThis.clearTimeout = originalClearTimeout + delete (globalThis as { window?: unknown }).window + delete (globalThis as { document?: unknown }).document + }) + + test("forced refresh replaces a merged PR with a newer open PR", async () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const newerOpen: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 2_000, + pr: { number: 15, title: "new", url: "u15", state: "open", draft: false, base: "main", head: "feature" }, + } + let requestCount = 0 + const github = { + prStatus: async () => { + requestCount += 1 + return requestCount === 1 ? merged : newerOpen + }, + } as unknown as RuntimeAPIs["github"] + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.getState().setParams(key, params(github, "feature")) + + await useGitHubPrStatusStore.getState().refresh(key, { force: true }) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(12) + + await useGitHubPrStatusStore.getState().refresh(key, { force: true }) + expect(requestCount).toBe(2) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(15) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.state).toBe("open") + }) + + test("forced refresh clears a merged PR when no open PR remains", async () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + repo: { owner: "acme", repo: "app", url: "https://github.com/acme/app" }, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const empty: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 2_000, + repo: { owner: "acme", repo: "app", url: "https://github.com/acme/app" }, + pr: null, + } + const github = { + prStatus: async () => empty, + } as unknown as RuntimeAPIs["github"] + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.setState((state) => ({ + entries: { + ...state.entries, + [key]: { + ...state.entries[key]!, + status: merged, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + }, + }, + })) + useGitHubPrStatusStore.getState().setParams(key, params(github, "feature")) + + await useGitHubPrStatusStore.getState().refresh(key, { force: true }) + + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr).toBeNull() + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.repo).toEqual({ + owner: "acme", + repo: "app", + url: "https://github.com/acme/app", + }) + }) + + test("watcher discovery revalidates a cached merged PR", async () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const newerOpen: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 2_000, + pr: { number: 15, title: "new", url: "u15", state: "open", draft: false, base: "main", head: "feature" }, + } + const responses = [merged, newerOpen] + let requestCount = 0 + const github = { + prStatus: async () => { + requestCount += 1 + return responses.shift()! + }, + } as unknown as RuntimeAPIs["github"] + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.getState().setParams(key, params(github, "feature")) + useGitHubPrStatusStore.getState().startWatching(key) + + for (let i = 0; i < 50 && useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number !== 12; i += 1) { + await Promise.resolve() + } + expect(requestCount).toBe(1) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(12) + expect(intervalCallbacks).toHaveLength(1) + + // Discovery poll for terminal state (lastDiscoveryPollAt starts at 0). + intervalCallbacks[0]!() + for (let i = 0; i < 50 && useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number !== 15; i += 1) { + await Promise.resolve() + } + + expect(requestCount).toBe(2) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(15) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.state).toBe("open") + }) + + test("watcher discovery clears a cached merged PR when no open PR exists", async () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const empty: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 2_000, + pr: null, + } + const responses = [merged, empty] + let requestCount = 0 + const github = { + prStatus: async () => { + requestCount += 1 + return responses.shift()! + }, + } as unknown as RuntimeAPIs["github"] + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.getState().setParams(key, params(github, "feature")) + useGitHubPrStatusStore.getState().startWatching(key) + + for (let i = 0; i < 50 && useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number !== 12; i += 1) { + await Promise.resolve() + } + expect(requestCount).toBe(1) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(12) + + intervalCallbacks[0]!() + for (let i = 0; i < 50 && useGitHubPrStatusStore.getState().entries[key]?.status?.pr != null; i += 1) { + await Promise.resolve() + } + + expect(requestCount).toBe(2) + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr).toBeNull() + }) + + test("does not seed sibling entries from a closed PR", () => { + const closed: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 9, title: "closed", url: "u9", state: "closed", draft: false, base: "main", head: "feature" }, + } + const autoKey = getGitHubPrStatusKey("/repo", "feature", null) + const originKey = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.setState({ + entries: { + [autoKey]: { + status: closed, + isLoading: false, + error: null, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + lastDiscoveryPollAt: 0, + watchers: 0, + params: null, + identity: { + runtimeKey: "runtime-a", + directory: "/repo", + branch: "feature", + remoteName: null, + }, + resolvedRemoteName: "origin", + paramsRevision: 0, + }, + }, + activeRequestCount: 0, + totalRequestCount: 0, + }) + + useGitHubPrStatusStore.getState().ensureEntry(originKey) + expect(useGitHubPrStatusStore.getState().entries[originKey]?.status).toBeNull() + expect(useGitHubPrStatusStore.getState().entries[originKey]?.isInitialStatusResolved).toBe(false) + }) +}) diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.ts b/packages/ui/src/stores/useGitHubPrStatusStore.ts index 32307518..2ff56460 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.ts @@ -212,6 +212,11 @@ const findResolvedSiblingEntry = ( if (entryKey === key || !entry.isInitialStatusResolved || !entry.status) { continue; } + // Never seed a fresh key from a closed/merged association — that is what + // made stale terminal PRs reappear after remote-key switches. + if (isTerminalPrState(entry.status.pr?.state)) { + continue; + } const parsed = parseStatusKey(entryKey); if (!parsed || parsed.runtimeKey !== target.runtimeKey @@ -359,15 +364,39 @@ const toPersistedEntry = (entry: PrStatusEntry): PersistedPrStatusEntry => ({ resolvedRemoteName: entry.resolvedRemoteName ?? entry.status?.resolvedRemoteName ?? null, }); -const hydrateEntry = (entry: PersistedPrStatusEntry | undefined): PrStatusEntry => ({ - ...createEntry(), - status: entry?.status ?? null, - isInitialStatusResolved: entry?.isInitialStatusResolved ?? false, - lastRefreshAt: entry?.lastRefreshAt ?? 0, - lastDiscoveryPollAt: entry?.lastDiscoveryPollAt ?? 0, - identity: entry?.identity ?? null, - resolvedRemoteName: entry?.resolvedRemoteName ?? entry?.status?.resolvedRemoteName ?? null, -}); +const stripTerminalPersistedStatus = ( + status: GitHubPullRequestStatus | null | undefined, +): GitHubPullRequestStatus | null => { + if (!status) { + return null; + } + if (!isTerminalPrState(status.pr?.state)) { + return status; + } + // Persisted closed/merged branch associations are not live authority. Keep + // repo/remote continuity so refresh can resume without briefly showing the + // stale terminal PR. + return { + ...status, + pr: null, + checks: undefined, + canMerge: undefined, + }; +}; + +const hydrateEntry = (entry: PersistedPrStatusEntry | undefined): PrStatusEntry => { + const status = stripTerminalPersistedStatus(entry?.status); + const hadTerminalPr = Boolean(entry?.status?.pr) && !status?.pr; + return { + ...createEntry(), + status, + isInitialStatusResolved: hadTerminalPr ? false : (entry?.isInitialStatusResolved ?? false), + lastRefreshAt: entry?.lastRefreshAt ?? 0, + lastDiscoveryPollAt: entry?.lastDiscoveryPollAt ?? 0, + identity: entry?.identity ?? null, + resolvedRemoteName: entry?.resolvedRemoteName ?? entry?.status?.resolvedRemoteName ?? null, + }; +}; const boundEntries = (entries: Record): Record => { const all = Object.entries(entries); @@ -496,7 +525,11 @@ export const useGitHubPrStatusStore = create()( } const hasPr = Boolean(entry.status?.pr); - if (!hasPr) { + const isTerminal = isTerminalPrState(entry.status?.pr?.state); + // Missing PR and terminal (closed/merged) PRs both need discovery: + // a new open PR may exist for the same head, or the association may + // need to clear to an authoritative empty result. + if (!hasPr || isTerminal) { const now = Date.now(); if (now - entry.lastDiscoveryPollAt < PR_DISCOVERY_INTERVAL_MS) { return; @@ -520,10 +553,6 @@ export const useGitHubPrStatusStore = create()( return; } - if (isTerminalPrState(entry.status?.pr?.state)) { - return; - } - const elapsed = Date.now() - entry.lastRefreshAt; const nextInterval = getOpenPrRefreshInterval(entry.status); if (elapsed < nextInterval) { @@ -850,6 +879,11 @@ export const useGitHubPrStatusStore = create()( if (!identity?.directory || !identity.branch) { return false; } + // Do not persist closed/merged branch associations — they become + // permanently sticky without a discovery refresh after reload. + if (isTerminalPrState(entry.status?.pr?.state)) { + return false; + } const freshness = Math.max(entry.lastRefreshAt, entry.lastDiscoveryPollAt); return freshness > 0 && Date.now() - freshness < PR_PERSIST_TTL_MS; }) diff --git a/packages/web/server/lib/github/DOCUMENTATION.md b/packages/web/server/lib/github/DOCUMENTATION.md index 4b7ce46e..c1ed499a 100644 --- a/packages/web/server/lib/github/DOCUMENTATION.md +++ b/packages/web/server/lib/github/DOCUMENTATION.md @@ -75,8 +75,9 @@ - It resolves those remotes into GitHub repos. - It expands each repo through `parent` and `source` so PRs in upstream repos can still be found. - It skips PR lookup when the current branch matches that repo's default branch. -- It first searches for PRs by likely source owner plus exact head branch. -- If that fails, it falls back to broader GitHub search for the branch name. +- 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. - `403` and `404` during repo lookups are treated as expected gaps, not hard errors. ## Shared client state model @@ -108,11 +109,16 @@ - Open PR with pending checks -> refresh about every `1m`. - Open PR with non-pending checks -> refresh about every `5m`. - Open PR without a stable checks signal -> refresh about every `2m`. -- Closed or merged PR -> stop regular polling. +- Closed or merged PR -> discovery refresh every `5m` (do not permanently stop polling). - Hidden tab -> skip polling. - Non-forced refreshes use a `90s` TTL. - Failed non-forced attempts also observe the `90s` TTL so transient server or rate-limit failures cannot retry on every sidebar update. Forced user/action refreshes bypass this guard. +## 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. + ## Background tracking rules - Track up to `50` likely directories. diff --git a/packages/web/server/lib/github/pr-status.js b/packages/web/server/lib/github/pr-status.js index f0f8c047..523f12d6 100644 --- a/packages/web/server/lib/github/pr-status.js +++ b/packages/web/server/lib/github/pr-status.js @@ -440,61 +440,62 @@ const searchFallbackPr = async ({ octokit, branch, repoNames }) => { const normalizedRepoNames = new Set(repoNames.map((name) => normalizeLower(name)).filter(Boolean)); - for (const state of ['open', 'closed']) { - let response; + // Branch status only discovers open PRs. Closed/merged history belongs to + // explicit PR list/detail workflows, not automatic branch association. + let response; + try { + response = await octokit.rest.search.issuesAndPullRequests({ + q: `is:pr state:open head:${branch}`, + per_page: 20, + }); + // If we get here, search API works for this repo — clear the disabled flag + _searchApiDisabledRepos.delete(repoKey); + } catch (error) { + noteIfGitHubRateLimit(error); + if (error?.status === 403) { + _searchApiDisabledRepos.set(repoKey, Date.now()); + return null; + } + if (error?.status === 404) { + rememberSearchMiss(missKey); + return null; + } + throw error; + } + + const items = Array.isArray(response?.data?.items) ? response.data.items : []; + for (const item of items) { + const repo = parseRepoFromApiUrl(item?.repository_url); + if (!repo) { + continue; + } + if (normalizedRepoNames.size > 0 && !normalizedRepoNames.has(normalizeLower(repo.repo))) { + continue; + } try { - response = await octokit.rest.search.issuesAndPullRequests({ - q: `is:pr state:${state} head:${branch}`, - per_page: 20, + const prResponse = await octokit.rest.pulls.get({ + owner: repo.owner, + repo: repo.repo, + pull_number: item.number, }); - // If we get here, search API works for this repo — clear the disabled flag - _searchApiDisabledRepos.delete(repoKey); - } catch (error) { - noteIfGitHubRateLimit(error); - if (error?.status === 403) { - _searchApiDisabledRepos.set(repoKey, Date.now()); - return null; + const pr = prResponse?.data; + if (!pr || normalizeText(pr.head?.ref) !== branch) { + continue; } - if (error?.status === 404) { + return { + repo: { + owner: repo.owner, + repo: repo.repo, + url: `https://github.com/${repo.owner}/${repo.repo}`, + }, + pr, + }; + } catch (error) { + if (error?.status === 403 || error?.status === 404) { continue; } throw error; } - - const items = Array.isArray(response?.data?.items) ? response.data.items : []; - for (const item of items) { - const repo = parseRepoFromApiUrl(item?.repository_url); - if (!repo) { - continue; - } - if (normalizedRepoNames.size > 0 && !normalizedRepoNames.has(normalizeLower(repo.repo))) { - continue; - } - try { - const prResponse = await octokit.rest.pulls.get({ - owner: repo.owner, - repo: repo.repo, - pull_number: item.number, - }); - const pr = prResponse?.data; - if (!pr || normalizeText(pr.head?.ref) !== branch) { - continue; - } - return { - repo: { - owner: repo.owner, - repo: repo.repo, - url: `https://github.com/${repo.owner}/${repo.repo}`, - }, - pr, - }; - } catch (error) { - if (error?.status === 403 || error?.status === 404) { - continue; - } - throw error; - } - } } rememberSearchMiss(missKey); @@ -511,24 +512,25 @@ 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; - for (const state of ['open', 'closed']) { - // Shared per-repo list first: one pulls.list answers every branch of the - // repo within the TTL. A miss in a complete list is authoritative — skip - // the per-branch query fan entirely. - let listWasComplete = false; - try { - const listEntry = await getRepoPulls(octokit, target.repo, state, { force }); - const fromList = pickPreferred(listEntry.prs); - if (fromList) { - return fromList; - } - listWasComplete = listEntry.complete; - } catch { - // fall through to the precise per-branch queries - } - if (listWasComplete) { - continue; + // 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. + const state = 'open'; + // Shared per-repo list first: one pulls.list answers every branch of the + // repo within the TTL. A miss in a complete list is authoritative — skip + // the per-branch query fan entirely. + let listWasComplete = false; + try { + const listEntry = await getRepoPulls(octokit, target.repo, state, { force }); + const fromList = pickPreferred(listEntry.prs); + if (fromList) { + return fromList; } + listWasComplete = listEntry.complete; + } catch { + // fall through to the precise per-branch queries + } + if (!listWasComplete) { if (coverage) { coverage.authoritative = false; } @@ -551,6 +553,9 @@ const findFirstMatchingPr = async ({ octokit, target, branch, sourceCandidates, return null; }; +// Exported for focused unit tests of open-only branch matching. +export { findFirstMatchingPr }; + export async function resolveGitHubPrStatus({ octokit, directory, branch, remoteName, force = false }) { // A deleted worktree can still have a session in the sidebar that keeps // requesting its PR status. Bail before touching git or GitHub for a diff --git a/packages/web/server/lib/github/pr-status.test.js b/packages/web/server/lib/github/pr-status.test.js new file mode 100644 index 00000000..14bc8bfb --- /dev/null +++ b/packages/web/server/lib/github/pr-status.test.js @@ -0,0 +1,89 @@ +import { beforeEach, describe, expect, mock, test } from 'bun:test'; + +const listMock = mock(async () => ({ data: [] })); + +mock.module('../git/index.js', () => ({ + getRemotes: async () => [], + getStatus: async () => null, +})); + +mock.module('./repo/index.js', () => ({ + resolveGitHubRepoFromDirectory: async () => null, +})); + +mock.module('./rate-limit.js', () => ({ + noteIfGitHubRateLimit: () => {}, +})); + +const { findFirstMatchingPr, invalidateRepoPullsCache } = await import('./pr-status.js'); + +const openPr = { + number: 15, + state: 'open', + head: { + ref: 'feature', + label: 'acme:feature', + user: { login: 'acme' }, + repo: { owner: { login: 'acme' }, name: 'app' }, + }, +}; + +const closedPr = { + number: 12, + state: 'closed', + merged_at: '2026-01-01T00:00:00Z', + head: { + ref: 'feature', + label: 'acme:feature', + user: { login: 'acme' }, + repo: { owner: { login: 'acme' }, name: 'app' }, + }, +}; + +describe('findFirstMatchingPr open-only branch status', () => { + 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); + }); + + 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] }; + }); + + 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).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); + }); +}); From 5e9e35897f28ad54a6ab0884697a7b4d193822f2 Mon Sep 17 00:00:00 2001 From: Serhii Dziupin Date: Fri, 14 Aug 2026 18:57:10 +0000 Subject: [PATCH 2/5] fix(github): harden terminal PR revalidation after critical review Recompute focus/visibility staleness at event time, retry closed/merged sidebar associations on the no-PR cadence, and assert refresh failures preserve prior PR status. Co-authored-by: Serhii Dziupin --- .../src/components/session/SessionSidebar.tsx | 8 +++-- .../views/git/PullRequestSection.tsx | 14 +++++--- .../src/stores/useGitHubPrStatusStore.test.ts | 32 +++++++++++++++++++ .../ui/src/stores/useGitHubPrStatusStore.ts | 2 ++ 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/packages/ui/src/components/session/SessionSidebar.tsx b/packages/ui/src/components/session/SessionSidebar.tsx index 2a2707c8..1d14494f 100644 --- a/packages/ui/src/components/session/SessionSidebar.tsx +++ b/packages/ui/src/components/session/SessionSidebar.tsx @@ -1471,12 +1471,16 @@ const SessionSidebarComponent: React.FC = ({ } const key = getGitHubPrStatusKey(directory, branch); const entry = useGitHubPrStatusStore.getState().entries[key]; - const hasPr = Boolean(entry?.status?.pr); + const prState = entry?.status?.pr?.state; + const isTerminalPr = prState === 'closed' || prState === 'merged'; + // Closed/merged associations are not live branch status — retry them on + // the same cadence as missing PRs so a newer open PR can appear. + const hasLivePr = Boolean(entry?.status?.pr) && !isTerminalPr; const retryKey = `${directory}::${branch}`; const noPrLastCheckedAt = Math.max(entry?.lastRefreshAt ?? 0, entry?.lastDiscoveryPollAt ?? 0); const shouldRetryNoPr = Boolean( entry?.isInitialStatusResolved - && !hasPr + && !hasLivePr && ( !retriedNoPrStatusKeysRef.current.has(retryKey) || now - noPrLastCheckedAt >= SIDEBAR_PR_NO_PR_RETRY_MS diff --git a/packages/ui/src/components/views/git/PullRequestSection.tsx b/packages/ui/src/components/views/git/PullRequestSection.tsx index 19a299c0..f85727ad 100644 --- a/packages/ui/src/components/views/git/PullRequestSection.tsx +++ b/packages/ui/src/components/views/git/PullRequestSection.tsx @@ -1169,16 +1169,20 @@ export const PullRequestSection: React.FC<{ React.useEffect(() => { // Terminal (closed/merged) status must still revalidate on focus/visibility: // the branch may now have a newer open PR, or the association may clear. - const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; - const isStale = Date.now() - lastRefreshAt > 60_000; - + // Recompute staleness inside the handlers — a captured boolean freezes after + // the first fresh refresh until lastRefreshAt changes again. const onFocus = () => { - if (isStale) { + const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; + if (Date.now() - lastRefreshAt > 60_000) { void refresh({ force: true, silent: true }); } }; const onVisibility = () => { - if (document.visibilityState === 'visible' && isStale) { + if (document.visibilityState !== 'visible') { + return; + } + const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; + if (Date.now() - lastRefreshAt > 60_000) { void refresh({ force: true, silent: true }); } }; diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts index 61ee2198..f8affe7c 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts @@ -408,4 +408,36 @@ describe("GitHub PR status stale terminal associations", () => { expect(useGitHubPrStatusStore.getState().entries[originKey]?.status).toBeNull() expect(useGitHubPrStatusStore.getState().entries[originKey]?.isInitialStatusResolved).toBe(false) }) + + test("keeps a cached PR when a forced refresh fails", async () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const github = { + prStatus: async () => { + throw new Error("GitHub unavailable") + }, + } as unknown as RuntimeAPIs["github"] + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.setState((state) => ({ + entries: { + ...state.entries, + [key]: { + ...state.entries[key]!, + status: merged, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + }, + }, + })) + useGitHubPrStatusStore.getState().setParams(key, params(github, "feature")) + + await useGitHubPrStatusStore.getState().refresh(key, { force: true }) + + expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(12) + expect(useGitHubPrStatusStore.getState().entries[key]?.error).toBe("GitHub unavailable") + }) }) diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.ts b/packages/ui/src/stores/useGitHubPrStatusStore.ts index 2ff56460..e8583a44 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.ts @@ -501,6 +501,8 @@ export const useGitHubPrStatusStore = create()( if (!entry || entry.watchers <= 0) { return; } + // Bootstrap retries only help discovery before any PR is known. Once a + // terminal PR is cached, the discovery interval owns revalidation. if (entry.status?.pr) { return; } From 13cbbc76d7ac4ff56581642bddfd111c6da11d74 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 04:27:52 +0000 Subject: [PATCH 3/5] test(github): cover persist, hydrate, and complete open-list misses Add store persist/hydrate regressions for terminal branch associations and a server test that a complete empty open list does not query closed PRs. Inline the open-only matcher state so the next repo target can win. Co-authored-by: serkraser --- .../src/stores/useGitHubPrStatusStore.test.ts | 100 ++++++++++++++++++ packages/web/server/lib/github/pr-status.js | 11 +- .../web/server/lib/github/pr-status.test.js | 16 +++ 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts index f8affe7c..351cf765 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts @@ -440,4 +440,104 @@ describe("GitHub PR status stale terminal associations", () => { expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr?.number).toBe(12) expect(useGitHubPrStatusStore.getState().entries[key]?.error).toBe("GitHub unavailable") }) + + test("does not persist a merged branch association", () => { + const merged: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + } + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.getState().setParams(key, params({} as RuntimeAPIs["github"], "feature")) + useGitHubPrStatusStore.setState((state) => ({ + entries: { + ...state.entries, + [key]: { + ...state.entries[key]!, + status: merged, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + }, + }, + })) + + const persisted = useGitHubPrStatusStore.persist.getOptions().partialize?.( + useGitHubPrStatusStore.getState(), + ) as { entries?: Record } | undefined + expect(persisted?.entries?.[key]).toBeUndefined() + }) + + test("still persists an open branch association", () => { + const open: GitHubPullRequestStatus = { + connected: true, + fetchedAt: 1_000, + pr: { number: 15, title: "new", url: "u15", state: "open", draft: false, base: "main", head: "feature" }, + } + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + useGitHubPrStatusStore.getState().ensureEntry(key) + useGitHubPrStatusStore.getState().setParams(key, params({} as RuntimeAPIs["github"], "feature")) + useGitHubPrStatusStore.setState((state) => ({ + entries: { + ...state.entries, + [key]: { + ...state.entries[key]!, + status: open, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + }, + }, + })) + + const persisted = useGitHubPrStatusStore.persist.getOptions().partialize?.( + useGitHubPrStatusStore.getState(), + ) as { entries?: Record } | undefined + expect(persisted?.entries?.[key]?.status?.pr?.number).toBe(15) + }) + + test("hydrate strips a legacy persisted merged PR and marks it unresolved", () => { + const key = getGitHubPrStatusKey("/repo", "feature", "origin") + const hydrated = useGitHubPrStatusStore.persist.getOptions().merge?.( + { + entries: { + [key]: { + status: { + connected: true, + fetchedAt: 1_000, + repo: { owner: "acme", repo: "app", url: "https://github.com/acme/app" }, + pr: { number: 12, title: "old", url: "u12", state: "merged", draft: false, base: "main", head: "feature" }, + checks: { state: "success", total: 1, success: 1, failure: 0, pending: 0 }, + canMerge: true, + }, + isInitialStatusResolved: true, + lastRefreshAt: Date.now(), + lastDiscoveryPollAt: 0, + identity: { + runtimeKey: "runtime-a", + directory: "/repo", + branch: "feature", + remoteName: "origin", + }, + resolvedRemoteName: "origin", + }, + }, + }, + useGitHubPrStatusStore.getState(), + ) as { + entries: Record + } + + expect(hydrated.entries[key]?.status?.pr).toBeNull() + expect(hydrated.entries[key]?.status?.repo).toEqual({ + owner: "acme", + repo: "app", + url: "https://github.com/acme/app", + }) + expect(hydrated.entries[key]?.status?.checks).toBeUndefined() + expect(hydrated.entries[key]?.status?.canMerge).toBeUndefined() + expect(hydrated.entries[key]?.isInitialStatusResolved).toBe(false) + }) }) diff --git a/packages/web/server/lib/github/pr-status.js b/packages/web/server/lib/github/pr-status.js index 523f12d6..f31cb30c 100644 --- a/packages/web/server/lib/github/pr-status.js +++ b/packages/web/server/lib/github/pr-status.js @@ -514,14 +514,11 @@ const findFirstMatchingPr = async ({ octokit, target, branch, sourceCandidates, // 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. - const state = 'open'; - // Shared per-repo list first: one pulls.list answers every branch of the - // repo within the TTL. A miss in a complete list is authoritative — skip - // the per-branch query fan entirely. + // 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; try { - const listEntry = await getRepoPulls(octokit, target.repo, state, { force }); + const listEntry = await getRepoPulls(octokit, target.repo, 'open', { force }); const fromList = pickPreferred(listEntry.prs); if (fromList) { return fromList; @@ -539,7 +536,7 @@ const findFirstMatchingPr = async ({ octokit, target, branch, sourceCandidates, const directCandidates = await safeListPulls(octokit, { owner: target.repo.owner, repo: target.repo.repo, - state, + state: 'open', head: `${owner}:${branch}`, per_page: 100, }); diff --git a/packages/web/server/lib/github/pr-status.test.js b/packages/web/server/lib/github/pr-status.test.js index 14bc8bfb..a481deca 100644 --- a/packages/web/server/lib/github/pr-status.test.js +++ b/packages/web/server/lib/github/pr-status.test.js @@ -86,4 +86,20 @@ describe('findFirstMatchingPr open-only branch status', () => { expect(listMock.mock.calls.every((call) => call[0]?.state === 'open')).toBe(true); expect(listMock.mock.calls.some((call) => call[0]?.state === 'closed')).toBe(false); }); + + test('does not query closed PRs when the open list is complete and empty', 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, + }); + + expect(pr).toBeNull(); + expect(listMock.mock.calls).toHaveLength(1); + expect(listMock.mock.calls[0]?.[0]?.state).toBe('open'); + }); }); From c1640522ece3dcd3de545570f52de58485e682f7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 04:27:52 +0000 Subject: [PATCH 4/5] docs(changelog): note stale merged PR panel fix Co-authored-by: serkraser --- CHANGELOG.md | 1 + packages/vscode/CHANGELOG.md | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a980e698..ec22a2ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Git: the pull request panel now follows the current open PR for the branch instead of keeping a merged or closed one after reload or a later open PR (thanks to @makeittech). - **Usage/Claude:** Claude plan limits now work when you are signed in through Claude Code, without also signing into Anthropic in OpenCode; the account is read from Claude Code's own login on macOS, Linux, and WSL. The page shows your session and weekly limits again, adds per-model weekly limits and extra usage spending, and names your plan. Limits are kept on screen instead of disappearing when Anthropic temporarily blocks refreshes. - **Settings/Integrations:** a new Integrations settings page lists Claude Code, Command Code, and Cursor plugins with install, update, setup, and remove actions, plus Discord and Telegram Coming soon placeholders. - Chat: opening a busy subagent in the context panel now shows its history instead of only the working-status line (thanks to @makeittech). diff --git a/packages/vscode/CHANGELOG.md b/packages/vscode/CHANGELOG.md index 8132a578..b80f1549 100644 --- a/packages/vscode/CHANGELOG.md +++ b/packages/vscode/CHANGELOG.md @@ -1,3 +1,7 @@ +## [Unreleased] + +- Git: the pull request panel now follows the current open PR for the branch instead of keeping a merged or closed one after reload or a later open PR (thanks to @makeittech). + ## [1.18.4] - 2026-08-14 - **Chat:** new messages now remain at the end of the conversation instead of jumping before older messages after the message ID sequence rolls over; history loading, revert, and redo follow the same chronological order. From ed972d9f9e6e154082c679b4da39b081deba45ef Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 04:30:30 +0000 Subject: [PATCH 5/5] test(ui): use toBe(undefined) for persist/hydrate assertions The UI expect helper does not type toBeUndefined. Co-authored-by: serkraser --- packages/ui/src/stores/useGitHubPrStatusStore.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts index 351cf765..ff02ff90 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts @@ -465,7 +465,7 @@ describe("GitHub PR status stale terminal associations", () => { const persisted = useGitHubPrStatusStore.persist.getOptions().partialize?.( useGitHubPrStatusStore.getState(), ) as { entries?: Record } | undefined - expect(persisted?.entries?.[key]).toBeUndefined() + expect(persisted?.entries?.[key]).toBe(undefined) }) test("still persists an open branch association", () => { @@ -536,8 +536,8 @@ describe("GitHub PR status stale terminal associations", () => { repo: "app", url: "https://github.com/acme/app", }) - expect(hydrated.entries[key]?.status?.checks).toBeUndefined() - expect(hydrated.entries[key]?.status?.canMerge).toBeUndefined() + expect(hydrated.entries[key]?.status?.checks).toBe(undefined) + expect(hydrated.entries[key]?.status?.canMerge).toBe(undefined) expect(hydrated.entries[key]?.isInitialStatusResolved).toBe(false) }) })