From 5e9e35897f28ad54a6ab0884697a7b4d193822f2 Mon Sep 17 00:00:00 2001 From: Serhii Dziupin Date: Fri, 14 Aug 2026 18:57:10 +0000 Subject: [PATCH] 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; }