diff --git a/CHANGELOG.md b/CHANGELOG.md index e3073032..2313f45a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +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). +- Git: the pull request panel now follows the branch's current open PR, and an open PR always wins over an older merged or closed one. After a PR is merged or closed the panel keeps showing it as the branch's last PR and offers creating the next one right below it (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: new chats no longer start against a deleted last worktree directory; they fall back to the active project instead of saving the first message and never starting. diff --git a/packages/ui/src/components/views/git/PullRequestSection.tsx b/packages/ui/src/components/views/git/PullRequestSection.tsx index f85727ad..3a258526 100644 --- a/packages/ui/src/components/views/git/PullRequestSection.tsx +++ b/packages/ui/src/components/views/git/PullRequestSection.tsx @@ -508,8 +508,13 @@ export const PullRequestSection: React.FC<{ }, [useDetectedUpstream, detectedUpstream?.defaultBranch]); const pr = status?.pr ?? null; + // A closed/merged PR is the branch's history, not its live status: it still + // deserves to be shown (you just merged it), but the branch is free again, so + // the panel offers creating the next PR instead of a read-only detail view. + const isHistoricalPr = pr?.state === 'merged' || pr?.state === 'closed'; + const livePr = isHistoricalPr ? null : pr; - const prContextKey = pr ? getPrContextKey(directory, pr.number) : null; + const prContextKey = livePr ? getPrContextKey(directory, livePr.number) : null; const prContextEntry = usePrContextStore((state) => (prContextKey ? state.entries[prContextKey] : undefined)); const ensurePrContext = usePrContextStore((state) => state.ensure); const prContext = prContextEntry?.result ?? null; @@ -525,14 +530,14 @@ export const PullRequestSection: React.FC<{ // Load the context the active segment needs; checks include details. React.useEffect(() => { - if (!pr || !github?.prContext || activeSegment === 'overview') { + if (!livePr || !github?.prContext || activeSegment === 'overview') { return; } - void ensurePrContext(github, directory, pr.number, { + void ensurePrContext(github, directory, livePr.number, { includeCheckDetails: activeSegment === 'checks', sourceRepo: status?.repo ?? null, }); - }, [activeSegment, directory, ensurePrContext, github, pr, status?.repo]); + }, [activeSegment, directory, ensurePrContext, github, livePr, status?.repo]); const checks = status?.checks ?? null; const checksArePending = (checks?.pending ?? 0) > 0; @@ -1167,12 +1172,11 @@ export const PullRequestSection: React.FC<{ }, [remotes, status?.resolvedRemoteName]); 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. - // Recompute staleness inside the handlers — a captured boolean freezes after - // the first fresh refresh until lastRefreshAt changes again. - const onFocus = () => { - const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; + // Coming back to the app is the moment a PR is most likely to have changed + // elsewhere — including a merged one being replaced by a newer open PR — so + // staleness is read from the store when the event fires, not captured here. + const refreshWhenStale = () => { + const lastRefreshAt = useGitHubPrStatusStore.getState().entries[prStatusKey]?.lastRefreshAt ?? 0; if (Date.now() - lastRefreshAt > 60_000) { void refresh({ force: true, silent: true }); } @@ -1181,19 +1185,16 @@ export const PullRequestSection: React.FC<{ if (document.visibilityState !== 'visible') { return; } - const lastRefreshAt = statusEntry?.lastRefreshAt ?? 0; - if (Date.now() - lastRefreshAt > 60_000) { - void refresh({ force: true, silent: true }); - } + refreshWhenStale(); }; - window.addEventListener('focus', onFocus); + window.addEventListener('focus', refreshWhenStale); document.addEventListener('visibilitychange', onVisibility); return () => { - window.removeEventListener('focus', onFocus); + window.removeEventListener('focus', refreshWhenStale); document.removeEventListener('visibilitychange', onVisibility); }; - }, [refresh, statusEntry?.lastRefreshAt]); + }, [prStatusKey, refresh]); React.useEffect(() => { if (githubAuthChecked && githubAuthStatus?.connected === false) { @@ -1614,7 +1615,7 @@ export const PullRequestSection: React.FC<{ {t('gitView.pr.checkingStatus')} - ) : pr ? ( + ) : pr && !isHistoricalPr ? (
) : (
+ {pr && isHistoricalPr ? ( +
+ +
+ {pr.state === 'merged' + ? t('gitView.pr.history.merged', { number: pr.number, base: pr.base || targetBaseBranch }) + : t('gitView.pr.history.closed', { number: pr.number })} +
+ +
+ ) : null}
{t('gitView.pr.createTitle')}
diff --git a/packages/ui/src/lib/i18n/messages/de.ts b/packages/ui/src/lib/i18n/messages/de.ts index 8c9f215d..fbb8a6cb 100644 --- a/packages/ui/src/lib/i18n/messages/de.ts +++ b/packages/ui/src/lib/i18n/messages/de.ts @@ -933,6 +933,8 @@ export const dict = { 'gitView.pr.field.draft': 'Entwurf', 'gitView.pr.field.title': 'Titel', 'gitView.pr.githubNotConnected': 'GitHub ist nicht verbunden', + 'gitView.pr.history.merged': 'PR #{number} wurde in {base} gemergt.', + 'gitView.pr.history.closed': 'PR #{number} wurde geschlossen.', 'gitView.pr.loadingDescription': 'Beschreibung wird geladen...', 'gitView.pr.mergeMethod.merge': 'Einen Merge-Commit erstellen', 'gitView.pr.mergeMethod.rebase': 'Rebase und Merge', diff --git a/packages/ui/src/lib/i18n/messages/en.ts b/packages/ui/src/lib/i18n/messages/en.ts index 9d60f0d7..52e6c94d 100644 --- a/packages/ui/src/lib/i18n/messages/en.ts +++ b/packages/ui/src/lib/i18n/messages/en.ts @@ -997,6 +997,8 @@ export const dict = { 'gitView.pr.field.draft': 'Draft', 'gitView.pr.field.title': 'Title', 'gitView.pr.githubNotConnected': 'GitHub is not connected', + 'gitView.pr.history.merged': 'PR #{number} was merged into {base}.', + 'gitView.pr.history.closed': 'PR #{number} was closed.', 'gitView.pr.loadingDescription': 'Loading description...', 'gitView.pr.mergeMethod.merge': 'Create a merge commit', 'gitView.pr.mergeMethod.rebase': 'Rebase and merge', diff --git a/packages/ui/src/lib/i18n/messages/es.ts b/packages/ui/src/lib/i18n/messages/es.ts index e30ea4ad..52e3b35f 100644 --- a/packages/ui/src/lib/i18n/messages/es.ts +++ b/packages/ui/src/lib/i18n/messages/es.ts @@ -998,6 +998,8 @@ export const dict: Record = { "gitView.pr.field.draft": "Borrador", "gitView.pr.field.title": "Título", "gitView.pr.githubNotConnected": "GitHub no está conectado", + "gitView.pr.history.merged": "La PR #{number} se fusionó en {base}.", + "gitView.pr.history.closed": "La PR #{number} se cerró.", "gitView.pr.loadingDescription": "Cargando descripción...", "gitView.pr.mergeMethod.merge": "Crear un merge commit", "gitView.pr.mergeMethod.rebase": "Rebase y merge", diff --git a/packages/ui/src/lib/i18n/messages/fr.ts b/packages/ui/src/lib/i18n/messages/fr.ts index f9c74dcf..9aed071b 100644 --- a/packages/ui/src/lib/i18n/messages/fr.ts +++ b/packages/ui/src/lib/i18n/messages/fr.ts @@ -817,6 +817,8 @@ export const dict = { 'gitView.pr.field.draft': 'Brouillon', 'gitView.pr.field.title': 'Titre', 'gitView.pr.githubNotConnected': 'GitHub n\'est pas connecté', + 'gitView.pr.history.merged': 'La PR #{number} a été fusionnée dans {base}.', + 'gitView.pr.history.closed': 'La PR #{number} a été fermée.', 'gitView.pr.loadingDescription': 'Chargement de la description de la PR...', 'gitView.pr.mergeMethod.merge': 'Créer un commit de fusion', 'gitView.pr.mergeMethod.rebase': 'Rebase et fusionner', diff --git a/packages/ui/src/lib/i18n/messages/ja.ts b/packages/ui/src/lib/i18n/messages/ja.ts index 441d7fd0..1d4959f7 100644 --- a/packages/ui/src/lib/i18n/messages/ja.ts +++ b/packages/ui/src/lib/i18n/messages/ja.ts @@ -994,6 +994,8 @@ export const dict: Record = { 'gitView.pr.field.draft': '下書き', 'gitView.pr.field.title': 'タイトル', 'gitView.pr.githubNotConnected': 'GitHubが接続されていません', + 'gitView.pr.history.merged': 'PR #{number} は {base} にマージされました。', + 'gitView.pr.history.closed': 'PR #{number} はクローズされました。', 'gitView.pr.loadingDescription': '説明を読み込み中...', 'gitView.pr.mergeMethod.merge': 'マージコミットを作成', 'gitView.pr.mergeMethod.rebase': 'リベースしてマージ', diff --git a/packages/ui/src/lib/i18n/messages/ko.ts b/packages/ui/src/lib/i18n/messages/ko.ts index 5c9499dd..52449bc8 100644 --- a/packages/ui/src/lib/i18n/messages/ko.ts +++ b/packages/ui/src/lib/i18n/messages/ko.ts @@ -998,6 +998,8 @@ export const dict: Record = { 'gitView.pr.field.draft': '드래프트', 'gitView.pr.field.title': '제목', 'gitView.pr.githubNotConnected': 'GitHub에 연결되지 않음', + 'gitView.pr.history.merged': 'PR #{number}이(가) {base}에 병합되었습니다.', + 'gitView.pr.history.closed': 'PR #{number}이(가) 닫혔습니다.', 'gitView.pr.loadingDescription': '설명 로드 중…', 'gitView.pr.mergeMethod.merge': '병합 커밋 생성', 'gitView.pr.mergeMethod.rebase': '리베이스 후 병합', diff --git a/packages/ui/src/lib/i18n/messages/pl.ts b/packages/ui/src/lib/i18n/messages/pl.ts index 54b53ee9..b903014d 100644 --- a/packages/ui/src/lib/i18n/messages/pl.ts +++ b/packages/ui/src/lib/i18n/messages/pl.ts @@ -2187,6 +2187,8 @@ export const dict: Record = { 'gitView.pr.field.draft': 'Szkic', 'gitView.pr.field.title': 'Tytuł', 'gitView.pr.githubNotConnected': 'GitHub nie jest połączony', + 'gitView.pr.history.merged': 'PR #{number} został scalony do {base}.', + 'gitView.pr.history.closed': 'PR #{number} został zamknięty.', 'gitView.pr.loadingDescription': 'Loading description...', 'gitView.pr.mergeMethod.merge': 'Create a merge commit', 'gitView.pr.mergeMethod.rebase': 'Rebase and merge', diff --git a/packages/ui/src/lib/i18n/messages/pt-BR.ts b/packages/ui/src/lib/i18n/messages/pt-BR.ts index 8f377453..500cc98b 100644 --- a/packages/ui/src/lib/i18n/messages/pt-BR.ts +++ b/packages/ui/src/lib/i18n/messages/pt-BR.ts @@ -998,6 +998,8 @@ export const dict: Record = { "gitView.pr.field.draft": "Borrador", "gitView.pr.field.title": "Título", "gitView.pr.githubNotConnected": "GitHub não está conectado", + "gitView.pr.history.merged": "A PR #{number} foi mesclada em {base}.", + "gitView.pr.history.closed": "A PR #{number} foi fechada.", "gitView.pr.loadingDescription": "Carregando descrição...", "gitView.pr.mergeMethod.merge": "Criar um merge commit", "gitView.pr.mergeMethod.rebase": "Rebase e merge", diff --git a/packages/ui/src/lib/i18n/messages/uk.ts b/packages/ui/src/lib/i18n/messages/uk.ts index d9f07823..fa1496f5 100644 --- a/packages/ui/src/lib/i18n/messages/uk.ts +++ b/packages/ui/src/lib/i18n/messages/uk.ts @@ -998,6 +998,8 @@ export const dict: Record = { "gitView.pr.field.draft": "Чернетка", "gitView.pr.field.title": "Назва", "gitView.pr.githubNotConnected": "GitHub не підключено", + "gitView.pr.history.merged": "PR #{number} злито в {base}.", + "gitView.pr.history.closed": "PR #{number} закрито.", "gitView.pr.loadingDescription": "Завантаження опису...", "gitView.pr.mergeMethod.merge": "Створити коміт злиття", "gitView.pr.mergeMethod.rebase": "Перебазувати та злити", diff --git a/packages/ui/src/lib/i18n/messages/zh-CN.ts b/packages/ui/src/lib/i18n/messages/zh-CN.ts index 7edfc6b3..2b670e97 100644 --- a/packages/ui/src/lib/i18n/messages/zh-CN.ts +++ b/packages/ui/src/lib/i18n/messages/zh-CN.ts @@ -998,6 +998,8 @@ export const dict: Record = { 'gitView.pr.field.draft': '草稿', 'gitView.pr.field.title': '标题', 'gitView.pr.githubNotConnected': 'GitHub 未连接', + 'gitView.pr.history.merged': 'PR #{number} 已合并到 {base}。', + 'gitView.pr.history.closed': 'PR #{number} 已关闭。', 'gitView.pr.loadingDescription': '正在加载描述...', 'gitView.pr.mergeMethod.merge': '创建合并提交', 'gitView.pr.mergeMethod.rebase': '变基并合并', diff --git a/packages/ui/src/lib/i18n/messages/zh-TW.ts b/packages/ui/src/lib/i18n/messages/zh-TW.ts index def005d2..313d59ed 100644 --- a/packages/ui/src/lib/i18n/messages/zh-TW.ts +++ b/packages/ui/src/lib/i18n/messages/zh-TW.ts @@ -1010,6 +1010,8 @@ export const dict: Record = { 'gitView.pr.field.draft': '草稿', 'gitView.pr.field.title': '標題', 'gitView.pr.githubNotConnected': 'GitHub 未連線', + 'gitView.pr.history.merged': 'PR #{number} 已合併到 {base}。', + 'gitView.pr.history.closed': 'PR #{number} 已關閉。', 'gitView.pr.loadingDescription': '正在載入描述...', 'gitView.pr.mergeMethod.merge': '建立合併提交', 'gitView.pr.mergeMethod.rebase': 'Rebase 並合併', diff --git a/packages/ui/src/stores/DOCUMENTATION.md b/packages/ui/src/stores/DOCUMENTATION.md index ee2295e0..7bd82bd1 100644 --- a/packages/ui/src/stores/DOCUMENTATION.md +++ b/packages/ui/src/stores/DOCUMENTATION.md @@ -173,10 +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 +- a closed/merged PR is the branch's history, not live status: it is displayed and persisted, but never treated as authority +- closed/merged associations use the same `5m` discovery cadence as missing PRs so a newer open PR (or authoritative `pr: null`) replaces them without a manual refresh +- hydrate restores a persisted closed/merged PR but resets its `lastDiscoveryPollAt`, so revalidation runs on the first watcher tick after a reload +- a successful refresh that returns `pr: null` replaces any previously cached PR authoritatively; a failed refresh keeps the previous one ## Ownership Rules diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts index ff02ff90..fb376750 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.test.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.test.ts @@ -371,7 +371,7 @@ describe("GitHub PR status stale terminal associations", () => { expect(useGitHubPrStatusStore.getState().entries[key]?.status?.pr).toBeNull() }) - test("does not seed sibling entries from a closed PR", () => { + test("seeds sibling entries from a closed PR without freezing discovery", () => { const closed: GitHubPullRequestStatus = { connected: true, fetchedAt: 1_000, @@ -405,8 +405,12 @@ describe("GitHub PR status stale terminal associations", () => { }) useGitHubPrStatusStore.getState().ensureEntry(originKey) - expect(useGitHubPrStatusStore.getState().entries[originKey]?.status).toBeNull() - expect(useGitHubPrStatusStore.getState().entries[originKey]?.isInitialStatusResolved).toBe(false) + const seeded = useGitHubPrStatusStore.getState().entries[originKey] + expect(seeded?.status?.pr?.number).toBe(9) + // Seeding is display continuity only: the seeded entry has never refreshed + // or polled, so its own discovery still runs immediately. + expect(seeded?.lastRefreshAt).toBe(0) + expect(seeded?.lastDiscoveryPollAt).toBe(0) }) test("keeps a cached PR when a forced refresh fails", async () => { @@ -441,7 +445,7 @@ describe("GitHub PR status stale terminal associations", () => { expect(useGitHubPrStatusStore.getState().entries[key]?.error).toBe("GitHub unavailable") }) - test("does not persist a merged branch association", () => { + test("persists a merged branch association as history", () => { const merged: GitHubPullRequestStatus = { connected: true, fetchedAt: 1_000, @@ -464,8 +468,8 @@ describe("GitHub PR status stale terminal associations", () => { const persisted = useGitHubPrStatusStore.persist.getOptions().partialize?.( useGitHubPrStatusStore.getState(), - ) as { entries?: Record } | undefined - expect(persisted?.entries?.[key]).toBe(undefined) + ) as { entries?: Record } | undefined + expect(persisted?.entries?.[key]?.status?.pr?.number).toBe(12) }) test("still persists an open branch association", () => { @@ -495,7 +499,7 @@ describe("GitHub PR status stale terminal associations", () => { expect(persisted?.entries?.[key]?.status?.pr?.number).toBe(15) }) - test("hydrate strips a legacy persisted merged PR and marks it unresolved", () => { + test("hydrate keeps a persisted merged PR but forces the next discovery poll", () => { const key = getGitHubPrStatusKey("/repo", "feature", "origin") const hydrated = useGitHubPrStatusStore.persist.getOptions().merge?.( { @@ -511,7 +515,7 @@ describe("GitHub PR status stale terminal associations", () => { }, isInitialStatusResolved: true, lastRefreshAt: Date.now(), - lastDiscoveryPollAt: 0, + lastDiscoveryPollAt: Date.now(), identity: { runtimeKey: "runtime-a", directory: "/repo", @@ -527,17 +531,19 @@ describe("GitHub PR status stale terminal associations", () => { entries: Record } - expect(hydrated.entries[key]?.status?.pr).toBeNull() + expect(hydrated.entries[key]?.status?.pr?.number).toBe(12) expect(hydrated.entries[key]?.status?.repo).toEqual({ owner: "acme", repo: "app", url: "https://github.com/acme/app", }) - expect(hydrated.entries[key]?.status?.checks).toBe(undefined) - expect(hydrated.entries[key]?.status?.canMerge).toBe(undefined) - expect(hydrated.entries[key]?.isInitialStatusResolved).toBe(false) + expect(hydrated.entries[key]?.isInitialStatusResolved).toBe(true) + // Restored history must not inherit a fresh discovery timestamp, otherwise + // a newer open PR would wait a full discovery interval after every reload. + expect(hydrated.entries[key]?.lastDiscoveryPollAt).toBe(0) }) }) diff --git a/packages/ui/src/stores/useGitHubPrStatusStore.ts b/packages/ui/src/stores/useGitHubPrStatusStore.ts index e8583a44..a2e82053 100644 --- a/packages/ui/src/stores/useGitHubPrStatusStore.ts +++ b/packages/ui/src/stores/useGitHubPrStatusStore.ts @@ -212,11 +212,6 @@ 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 @@ -364,35 +359,18 @@ const toPersistedEntry = (entry: PrStatusEntry): PersistedPrStatusEntry => ({ 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; + // A persisted closed/merged PR is restored so the panel keeps showing the + // branch's PR history across a reload. It is never treated as live authority: + // `lastDiscoveryPollAt` is reset so the watcher revalidates it immediately and + // an open PR (or an authoritative empty result) replaces it. + const hasTerminalPr = isTerminalPrState(entry?.status?.pr?.state); return { ...createEntry(), - status, - isInitialStatusResolved: hadTerminalPr ? false : (entry?.isInitialStatusResolved ?? false), + status: entry?.status ?? null, + isInitialStatusResolved: entry?.isInitialStatusResolved ?? false, lastRefreshAt: entry?.lastRefreshAt ?? 0, - lastDiscoveryPollAt: entry?.lastDiscoveryPollAt ?? 0, + lastDiscoveryPollAt: hasTerminalPr ? 0 : (entry?.lastDiscoveryPollAt ?? 0), identity: entry?.identity ?? null, resolvedRemoteName: entry?.resolvedRemoteName ?? entry?.status?.resolvedRemoteName ?? null, }; @@ -501,8 +479,9 @@ 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. + // Bootstrap retries only help discovery before any PR is known. + // Once a PR is cached — open or historical — the discovery interval + // owns revalidation. if (entry.status?.pr) { return; } @@ -527,10 +506,10 @@ export const useGitHubPrStatusStore = create()( } const hasPr = Boolean(entry.status?.pr); + // A closed/merged PR is history, not live status. It stays on the + // discovery cadence like a branch with no PR at all, so a newer open + // PR — or an authoritative empty result — replaces it on its own. 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) { @@ -881,11 +860,6 @@ 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/vscode/CHANGELOG.md b/packages/vscode/CHANGELOG.md index e97fd5f8..86d45514 100644 --- a/packages/vscode/CHANGELOG.md +++ b/packages/vscode/CHANGELOG.md @@ -1,7 +1,6 @@ ## [Unreleased] - Chat: new chats no longer start against a deleted last worktree directory; they fall back to the active project instead of saving the first message and never starting. -- 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 diff --git a/packages/web/server/lib/github/DOCUMENTATION.md b/packages/web/server/lib/github/DOCUMENTATION.md index c1ed499a..41d978e9 100644 --- a/packages/web/server/lib/github/DOCUMENTATION.md +++ b/packages/web/server/lib/github/DOCUMENTATION.md @@ -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 diff --git a/packages/web/server/lib/github/pr-status.js b/packages/web/server/lib/github/pr-status.js index f31cb30c..8e88122c 100644 --- a/packages/web/server/lib/github/pr-status.js +++ b/packages/web/server/lib/github/pr-status.js @@ -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, diff --git a/packages/web/server/lib/github/pr-status.test.js b/packages/web/server/lib/github/pr-status.test.js index a481deca..bbde6c78 100644 --- a/packages/web/server/lib/github/pr-status.test.js +++ b/packages/web/server/lib/github/pr-status.test.js @@ -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); + }); }); diff --git a/packages/web/server/lib/github/routes.js b/packages/web/server/lib/github/routes.js index 427348aa..c083c236 100644 --- a/packages/web/server/lib/github/routes.js +++ b/packages/web/server/lib/github/routes.js @@ -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,