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 <makeittech@users.noreply.github.com>
This commit is contained in:
committed by
Cursor Agent
co-authored by
Serhii Dziupin
parent
6b1e677aaf
commit
962b016cbd
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, PrStatusEntry>): Record<string, PrStatusEntry> => {
|
||||
const all = Object.entries(entries);
|
||||
@@ -496,7 +525,11 @@ export const useGitHubPrStatusStore = create<GitHubPrStatusStore>()(
|
||||
}
|
||||
|
||||
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<GitHubPrStatusStore>()(
|
||||
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<GitHubPrStatusStore>()(
|
||||
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;
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user