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 <makeittech@users.noreply.github.com>
This commit is contained in:
committed by
Cursor Agent
co-authored by
Serhii Dziupin
parent
962b016cbd
commit
5e9e35897f
@@ -1471,12 +1471,16 @@ const SessionSidebarComponent: React.FC<SessionSidebarProps> = ({
|
|||||||
}
|
}
|
||||||
const key = getGitHubPrStatusKey(directory, branch);
|
const key = getGitHubPrStatusKey(directory, branch);
|
||||||
const entry = useGitHubPrStatusStore.getState().entries[key];
|
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 retryKey = `${directory}::${branch}`;
|
||||||
const noPrLastCheckedAt = Math.max(entry?.lastRefreshAt ?? 0, entry?.lastDiscoveryPollAt ?? 0);
|
const noPrLastCheckedAt = Math.max(entry?.lastRefreshAt ?? 0, entry?.lastDiscoveryPollAt ?? 0);
|
||||||
const shouldRetryNoPr = Boolean(
|
const shouldRetryNoPr = Boolean(
|
||||||
entry?.isInitialStatusResolved
|
entry?.isInitialStatusResolved
|
||||||
&& !hasPr
|
&& !hasLivePr
|
||||||
&& (
|
&& (
|
||||||
!retriedNoPrStatusKeysRef.current.has(retryKey)
|
!retriedNoPrStatusKeysRef.current.has(retryKey)
|
||||||
|| now - noPrLastCheckedAt >= SIDEBAR_PR_NO_PR_RETRY_MS
|
|| now - noPrLastCheckedAt >= SIDEBAR_PR_NO_PR_RETRY_MS
|
||||||
|
|||||||
@@ -1169,16 +1169,20 @@ export const PullRequestSection: React.FC<{
|
|||||||
React.useEffect(() => {
|
React.useEffect(() => {
|
||||||
// Terminal (closed/merged) status must still revalidate on focus/visibility:
|
// Terminal (closed/merged) status must still revalidate on focus/visibility:
|
||||||
// the branch may now have a newer open PR, or the association may clear.
|
// the branch may now have a newer open PR, or the association may clear.
|
||||||
const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0;
|
// Recompute staleness inside the handlers — a captured boolean freezes after
|
||||||
const isStale = Date.now() - lastRefreshAt > 60_000;
|
// the first fresh refresh until lastRefreshAt changes again.
|
||||||
|
|
||||||
const onFocus = () => {
|
const onFocus = () => {
|
||||||
if (isStale) {
|
const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0;
|
||||||
|
if (Date.now() - lastRefreshAt > 60_000) {
|
||||||
void refresh({ force: true, silent: true });
|
void refresh({ force: true, silent: true });
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
const onVisibility = () => {
|
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 });
|
void refresh({ force: true, silent: true });
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -408,4 +408,36 @@ describe("GitHub PR status stale terminal associations", () => {
|
|||||||
expect(useGitHubPrStatusStore.getState().entries[originKey]?.status).toBeNull()
|
expect(useGitHubPrStatusStore.getState().entries[originKey]?.status).toBeNull()
|
||||||
expect(useGitHubPrStatusStore.getState().entries[originKey]?.isInitialStatusResolved).toBe(false)
|
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")
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -501,6 +501,8 @@ export const useGitHubPrStatusStore = create<GitHubPrStatusStore>()(
|
|||||||
if (!entry || entry.watchers <= 0) {
|
if (!entry || entry.watchers <= 0) {
|
||||||
return;
|
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) {
|
if (entry.status?.pr) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user