diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 38c89f37d..ce6d0acea 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -89,6 +89,10 @@ import { restorePRCommentThreadSnapshot } from './pr-comment-thread-resolution' import { installWindowVisibilityTimeoutPoller } from '@/lib/window-visibility-timeout-poller' +import { + CHECKS_PANEL_BASE_POLL_INTERVAL_MS, + nextChecksPanelPollInterval +} from './checks-panel-polling' import { getChecksPanelEmptyStateCopy, shouldShowChecksPanelPublishBranchAction @@ -449,7 +453,7 @@ export default function ChecksPanel(): React.JSX.Element { const [titleSaving, setTitleSaving] = useState(false) const titleInputRef = useRef(null) const titleInputFocusTimerRef = useRef | null>(null) - const pollIntervalRef = useRef(30_000) // start at 30s, backs off to 120s + const pollIntervalRef = useRef(CHECKS_PANEL_BASE_POLL_INTERVAL_MS) const mountedRef = useMountedRef() const confirm = useConfirmationDialog() const prevChecksRef = useRef('') @@ -584,7 +588,7 @@ export default function ChecksPanel(): React.JSX.Element { setHostedReviewCreationSnapshot(null) setGitStatusSnapshot(null) setGitStatusRefreshNonce((value) => value + 1) - pollIntervalRef.current = 30_000 + pollIntervalRef.current = CHECKS_PANEL_BASE_POLL_INTERVAL_MS prevChecksRef.current = '' conflictSummaryRefreshKeyRef.current = null refreshRequestKeyRef.current = null @@ -1362,14 +1366,13 @@ export default function ChecksPanel(): React.JSX.Element { } setChecks(result) - // Exponential backoff: if checks haven't changed, double the interval (cap 120s). - // If they changed, reset to 30s. - const signature = JSON.stringify(result.map((c) => `${c.name}:${c.status}:${c.conclusion}`)) - pollIntervalRef.current = - signature === prevChecksRef.current - ? Math.min(pollIntervalRef.current * 2, 120_000) - : 30_000 - prevChecksRef.current = signature + const poll = nextChecksPanelPollInterval({ + checks: result, + previousSignature: prevChecksRef.current, + currentIntervalMs: pollIntervalRef.current + }) + pollIntervalRef.current = poll.intervalMs + prevChecksRef.current = poll.signature } catch (err) { if ( !isCurrentAsyncResult( @@ -1442,12 +1445,13 @@ export default function ChecksPanel(): React.JSX.Element { const result = gitLabPipelineJobsToPRChecks(details?.pipelineJobs ?? []) setChecks(result) setComments(gitLabMRCommentsToPRComments(details?.comments)) - const signature = JSON.stringify(result.map((c) => `${c.name}:${c.status}:${c.conclusion}`)) - pollIntervalRef.current = - signature === prevChecksRef.current - ? Math.min(pollIntervalRef.current * 2, 120_000) - : 30_000 - prevChecksRef.current = signature + const poll = nextChecksPanelPollInterval({ + checks: result, + previousSignature: prevChecksRef.current, + currentIntervalMs: pollIntervalRef.current + }) + pollIntervalRef.current = poll.intervalMs + prevChecksRef.current = poll.signature } catch (err) { if (!isCurrentAsyncResult(requestKey)) { return @@ -1484,7 +1488,7 @@ export default function ChecksPanel(): React.JSX.Element { } // Reset backoff state on PR change - pollIntervalRef.current = 30_000 + pollIntervalRef.current = CHECKS_PANEL_BASE_POLL_INTERVAL_MS prevChecksRef.current = '' // Why: PR check status is user-visible when the panel is open. Keep visible // unfocused windows fresh, but stop timers and API work while hidden. @@ -1499,7 +1503,7 @@ export default function ChecksPanel(): React.JSX.Element { return } - pollIntervalRef.current = 30_000 + pollIntervalRef.current = CHECKS_PANEL_BASE_POLL_INTERVAL_MS prevChecksRef.current = '' return installWindowVisibilityTimeoutPoller({ run: () => fetchGitLabDetails(), @@ -1751,14 +1755,13 @@ export default function ChecksPanel(): React.JSX.Element { return } setChecks(result) - const signature = JSON.stringify( - result.map((c) => `${c.name}:${c.status}:${c.conclusion}`) - ) - pollIntervalRef.current = - signature === prevChecksRef.current - ? Math.min(pollIntervalRef.current * 2, 120_000) - : 30_000 - prevChecksRef.current = signature + const poll = nextChecksPanelPollInterval({ + checks: result, + previousSignature: prevChecksRef.current, + currentIntervalMs: pollIntervalRef.current + }) + pollIntervalRef.current = poll.intervalMs + prevChecksRef.current = poll.signature }, (err) => { if (!isCurrentRequest() || !isCurrentAsyncResult(prRequestKey)) { @@ -1836,9 +1839,8 @@ export default function ChecksPanel(): React.JSX.Element { return } // Why: entering the Checks tab is automatic UI behavior, not an explicit - // user refresh. Route PR refresh through the coordinator so rate-limit - // guards still apply; only force detail panes that the entry freshness rule - // already proved stale, so tab entry stays fresh without broad fan-out. + // user refresh. Keep check loads cacheable so the GitHub CLI cache can + // absorb visible polling; only the manual refresh button bypasses it. if (isGitLabReviewContext) { void fetchHostedReviewForBranch(repo.path, branch, { force: true, @@ -1854,7 +1856,7 @@ export default function ChecksPanel(): React.JSX.Element { } enqueueGitHubPRRefresh(activeWorktreeId, 'active', 80) if (options.refreshChecks) { - void fetchChecks({ force: true }) + void fetchChecks() } if (options.refreshComments) { void fetchComments({ force: true }) @@ -1918,9 +1920,9 @@ export default function ChecksPanel(): React.JSX.Element { const refreshComments = prNumber !== null && (commentsFetchedAt === undefined || commentsFetchedAt < cutoff) - // Reset polling attention state so the forced fetch's signature establishes - // a fresh baseline rather than colliding with the previous PR's backoff. - pollIntervalRef.current = 30_000 + // Reset polling attention state so this entry refresh establishes a fresh + // baseline rather than colliding with the previous PR's backoff. + pollIntervalRef.current = CHECKS_PANEL_BASE_POLL_INTERVAL_MS prevChecksRef.current = '' handleEntryRefresh({ refreshChecks, refreshComments }) }, [entryKey, prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, handleEntryRefresh]) @@ -3300,7 +3302,7 @@ export default function ChecksPanel(): React.JSX.Element { )} {/* Why: when the hosted review has merge conflicts and no checks have been fetched, - showing "No checks configured" is misleading — checks may exist but + showing an empty checks state is misleading — checks may exist but simply cannot run until conflicts are resolved. Hide the empty state. */} {!(activeConflictReview && checks.length === 0 && !checksLoading) && ( {translate( 'auto.components.right.sidebar.checks.panel.content.991f50c7e4', - 'No checks configured' + 'No checks reported yet' )} ) : !checksExpanded ? null : ( diff --git a/src/renderer/src/components/right-sidebar/checks-panel-polling.test.ts b/src/renderer/src/components/right-sidebar/checks-panel-polling.test.ts new file mode 100644 index 000000000..55e453ded --- /dev/null +++ b/src/renderer/src/components/right-sidebar/checks-panel-polling.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest' + +import type { PRCheckDetail } from '../../../../shared/types' +import { + CHECKS_PANEL_BASE_POLL_INTERVAL_MS, + CHECKS_PANEL_MAX_POLL_INTERVAL_MS, + nextChecksPanelPollInterval +} from './checks-panel-polling' + +describe('nextChecksPanelPollInterval', () => { + it('keeps repeated empty results at the baseline poll interval', () => { + expect( + nextChecksPanelPollInterval({ + checks: [], + previousSignature: '[]', + currentIntervalMs: CHECKS_PANEL_MAX_POLL_INTERVAL_MS + }) + ).toEqual({ intervalMs: CHECKS_PANEL_BASE_POLL_INTERVAL_MS, signature: '[]' }) + }) + + it('backs off repeated non-empty results up to the maximum interval', () => { + const checks: PRCheckDetail[] = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ] + const { signature } = nextChecksPanelPollInterval({ + checks, + previousSignature: '', + currentIntervalMs: CHECKS_PANEL_BASE_POLL_INTERVAL_MS + }) + + expect( + nextChecksPanelPollInterval({ + checks, + previousSignature: signature, + currentIntervalMs: CHECKS_PANEL_BASE_POLL_INTERVAL_MS + }).intervalMs + ).toBe(CHECKS_PANEL_BASE_POLL_INTERVAL_MS * 2) + expect( + nextChecksPanelPollInterval({ + checks, + previousSignature: signature, + currentIntervalMs: CHECKS_PANEL_MAX_POLL_INTERVAL_MS + }).intervalMs + ).toBe(CHECKS_PANEL_MAX_POLL_INTERVAL_MS) + }) + + it('resets changed non-empty results to the baseline poll interval', () => { + const previous: PRCheckDetail[] = [ + { name: 'build', status: 'queued', conclusion: null, url: null } + ] + const next: PRCheckDetail[] = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ] + const { signature } = nextChecksPanelPollInterval({ + checks: previous, + previousSignature: '', + currentIntervalMs: CHECKS_PANEL_BASE_POLL_INTERVAL_MS + }) + + expect( + nextChecksPanelPollInterval({ + checks: next, + previousSignature: signature, + currentIntervalMs: CHECKS_PANEL_MAX_POLL_INTERVAL_MS + }).intervalMs + ).toBe(CHECKS_PANEL_BASE_POLL_INTERVAL_MS) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-polling.ts b/src/renderer/src/components/right-sidebar/checks-panel-polling.ts new file mode 100644 index 000000000..425730795 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/checks-panel-polling.ts @@ -0,0 +1,26 @@ +import type { PRCheckDetail } from '../../../../shared/types' + +export const CHECKS_PANEL_BASE_POLL_INTERVAL_MS = 30_000 +export const CHECKS_PANEL_MAX_POLL_INTERVAL_MS = 120_000 + +export function nextChecksPanelPollInterval(input: { + checks: PRCheckDetail[] + previousSignature: string + currentIntervalMs: number +}): { intervalMs: number; signature: string } { + const signature = JSON.stringify( + input.checks.map((check) => `${check.name}:${check.status}:${check.conclusion}`) + ) + + if (input.checks.length === 0) { + return { intervalMs: CHECKS_PANEL_BASE_POLL_INTERVAL_MS, signature } + } + + return { + intervalMs: + signature === input.previousSignature + ? Math.min(input.currentIntervalMs * 2, CHECKS_PANEL_MAX_POLL_INTERVAL_MS) + : CHECKS_PANEL_BASE_POLL_INTERVAL_MS, + signature + } +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 6b2e7498b..e12518b19 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -8070,7 +8070,7 @@ "74c6885b8a": "More comment actions", "cbcc4ab3db": "Showing first 100 checks", "0dca6bfab5": "Open check details", - "991f50c7e4": "No checks configured", + "991f50c7e4": "No checks reported yet", "9ad98f2a17": "pending", "5e52f4ef7f": "failing", "02ca4f9074": "passing", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index e6f3a59db..3b33da37a 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -8070,7 +8070,7 @@ "74c6885b8a": "Más acciones de comentarios", "cbcc4ab3db": "Mostrando los primeros 100 cheques", "0dca6bfab5": "Abrir detalles del cheque", - "991f50c7e4": "No hay controles configurados", + "991f50c7e4": "Aún no se han reportado controles", "9ad98f2a17": "pendiente", "5e52f4ef7f": "defecto", "02ca4f9074": "paso", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 809a3162f..f44cf3ae5 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -8070,7 +8070,7 @@ "74c6885b8a": "その他のコメント操作", "cbcc4ab3db": "最初の 100 件のチェックを表示しています", "0dca6bfab5": "オープンチェックの詳細", - "991f50c7e4": "チェックが設定されていません", + "991f50c7e4": "まだチェックは報告されていません", "9ad98f2a17": "保留中", "5e52f4ef7f": "失敗した", "02ca4f9074": "合格", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 843af4c67..5ce1070f7 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -8070,7 +8070,7 @@ "74c6885b8a": "추가 댓글 작업", "cbcc4ab3db": "처음 100개 검사 표시", "0dca6bfab5": "검사 세부 정보 열기", - "991f50c7e4": "구성된 검사가 없습니다.", + "991f50c7e4": "아직 보고된 체크가 없습니다.", "9ad98f2a17": "보류 중", "5e52f4ef7f": "실패", "02ca4f9074": "통과", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 43beb2959..36c6a9f3b 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -8070,7 +8070,7 @@ "74c6885b8a": "更多评论动作", "cbcc4ab3db": "显示前 100 项检查", "0dca6bfab5": "打开检查详细信息", - "991f50c7e4": "未配置检查", + "991f50c7e4": "尚未报告任何检查", "9ad98f2a17": "待办的", "5e52f4ef7f": "失败", "02ca4f9074": "通过", diff --git a/src/renderer/src/store/slices/github-checks-cache.test.ts b/src/renderer/src/store/slices/github-checks-cache.test.ts new file mode 100644 index 000000000..2474ac521 --- /dev/null +++ b/src/renderer/src/store/slices/github-checks-cache.test.ts @@ -0,0 +1,390 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' + +import { createGitHubSlice, prChecksCacheSuffix } from './github' +import { createHostedReviewSlice } from './hosted-review' +import { getHostedReviewCacheKey } from './hosted-review-cache-identity' +import type { AppState } from '../types' +import type { PRCheckDetail, PRInfo } from '../../../../shared/types' + +const mockApi = { + gh: { + prChecks: vi.fn() + }, + cache: { + setGitHub: vi.fn() + } +} + +// @ts-expect-error test window mock +globalThis.window = { api: mockApi } + +type Deferred = { + promise: Promise + resolve: (value: T) => void +} + +function deferred(): Deferred { + let resolve: (value: T) => void = () => {} + const promise = new Promise((promiseResolve) => { + resolve = promiseResolve + }) + return { promise, resolve } +} + +function createTestStore() { + return create()( + (...a) => + ({ + ...createGitHubSlice(...a), + ...createHostedReviewSlice(...a) + }) as AppState + ) +} + +function makePR(overrides: Partial = {}): PRInfo { + return { + number: 12, + title: 'Test PR', + state: 'open', + url: 'https://example.com/pr/12', + checksStatus: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'head-oid', + ...overrides + } +} + +beforeEach(() => { + mockApi.gh.prChecks.mockReset() + mockApi.gh.prChecks.mockResolvedValue([]) + mockApi.cache.setGitHub.mockReset() + mockApi.cache.setGitHub.mockResolvedValue(undefined) +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('createGitHubSlice.fetchPRChecks checks cache freshness', () => { + it('expires empty checks cache entries after the shorter empty TTL', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + + mockApi.gh.prChecks.mockResolvedValue([]) + + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + vi.setSystemTime(11_001) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(2) + }) + + it('keeps repeated automatic empty checks refreshes cacheable', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + + mockApi.gh.prChecks.mockResolvedValue([]) + + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + vi.setSystemTime(11_001) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(2) + expect(mockApi.gh.prChecks).toHaveBeenNthCalledWith(1, { + repoPath, + repoId, + prNumber: 12, + headSha: undefined, + prRepo: null, + noCache: false + }) + expect(mockApi.gh.prChecks).toHaveBeenNthCalledWith(2, { + repoPath, + repoId, + prNumber: 12, + headSha: undefined, + prRepo: null, + noCache: false + }) + }) + + it('keeps non-empty checks cache entries fresh for the normal checks TTL', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + + mockApi.gh.prChecks.mockResolvedValue([ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ]) + + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + vi.setSystemTime(11_001) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(1) + }) + + it('dedupes simultaneous cacheable empty checks requests', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + const request = deferred() + mockApi.gh.prChecks.mockReturnValueOnce(request.promise) + + const first = store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + const second = store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(1) + request.resolve([]) + + await expect(first).resolves.toEqual([]) + await expect(second).resolves.toEqual([]) + }) + + it('does not dedupe forced checks onto an in-flight cacheable request', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + const cacheableRequest = deferred() + const forcedChecks = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } as const + ] + mockApi.gh.prChecks + .mockReturnValueOnce(cacheableRequest.promise) + .mockResolvedValueOnce(forcedChecks) + + const cacheable = store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + const forced = store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(1) + cacheableRequest.resolve([]) + + await expect(cacheable).resolves.toEqual([]) + await expect(forced).resolves.toEqual(forcedChecks) + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(2) + expect(mockApi.gh.prChecks).toHaveBeenNthCalledWith(2, { + repoPath, + repoId, + prNumber: 12, + headSha: undefined, + prRepo: null, + noCache: true + }) + }) + + it('dedupes simultaneous forced checks requests', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + const request = deferred() + const checks = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } as const + ] + mockApi.gh.prChecks.mockReturnValueOnce(request.promise) + + const first = store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) + const second = store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(1) + request.resolve(checks) + + await expect(first).resolves.toEqual(checks) + await expect(second).resolves.toEqual(checks) + expect(mockApi.gh.prChecks).toHaveBeenCalledWith({ + repoPath, + repoId, + prNumber: 12, + headSha: undefined, + prRepo: null, + noCache: true + }) + }) + + it('treats explicit noCache checks requests as fresh requests', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + + mockApi.gh.prChecks.mockResolvedValue([ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ]) + + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { noCache: true, repoId }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(2) + expect(mockApi.gh.prChecks).toHaveBeenNthCalledWith(2, { + repoPath, + repoId, + prNumber: 12, + headSha: undefined, + prRepo: null, + noCache: true + }) + }) + + it('preserves cached checks when the checks IPC fails', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const checksCacheKey = `${repoPath}::pr-checks::12` + const cachedChecks = [ + { name: 'build', status: 'completed', conclusion: 'failure', url: null } as const + ] + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: cachedChecks, + fetchedAt: 1, + headSha: 'abc123head' + } + } + } as unknown as Partial) + mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) + + await expect( + store.getState().fetchPRChecks(repoPath, 12, branch, 'abc123head', null, { force: true }) + ).resolves.toEqual(cachedChecks) + + expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(cachedChecks) + expect(store.getState().checksCache[checksCacheKey]?.fetchedAt).toBe(1) + }) + + it('does not return cached checks for a different requested head SHA after IPC failure', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const checksCacheKey = `${repoPath}::pr-checks::12` + const oldHeadChecks = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } as const + ] + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: oldHeadChecks, + fetchedAt: 1, + headSha: 'old-head' + } + } + } as unknown as Partial) + mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) + + await expect( + store.getState().fetchPRChecks(repoPath, 12, branch, 'new-head', null, { force: true }) + ).resolves.toEqual([]) + + expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(oldHeadChecks) + expect(store.getState().checksCache[checksCacheKey]?.headSha).toBe('old-head') + }) +}) + +describe('createGitHubSlice.applyGitHubPRRefreshEvent checks cache reuse', () => { + it('does not derive neutral PR status from stale empty checks during refresh events', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/stale-empty-checks' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const checksCacheKey = `${repoId}::${prChecksCacheSuffix(12, null, 'head-oid')}` + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: [], + fetchedAt: 1_000, + headSha: 'head-oid' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, checksStatus: 'pending', headSha: 'head-oid' }), + fetchedAt: 21_000 + } + }) + + expect(store.getState().prCache[cacheKey]).toMatchObject({ + data: expect.objectContaining({ checksStatus: 'pending' }), + fetchedAt: 21_000 + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ provider: 'github', status: 'pending' }), + fetchedAt: 21_000 + }) + }) + + it('reuses fresh head-specific checks cache entries during refresh events', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/fresh-head-checks' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const checksCacheKey = `${repoId}::${prChecksCacheSuffix(12, null, 'head-oid')}` + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: [{ name: 'build', status: 'completed', conclusion: 'success', url: null }], + fetchedAt: 20_000, + headSha: 'head-oid' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, checksStatus: 'pending', headSha: 'head-oid' }), + fetchedAt: 21_000 + } + }) + + expect(store.getState().prCache[cacheKey]).toMatchObject({ + data: expect.objectContaining({ checksStatus: 'success' }), + fetchedAt: 21_000 + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ provider: 'github', status: 'success' }), + fetchedAt: 21_000 + }) + }) +}) diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 10d9bf92f..c45ecf558 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -847,62 +847,6 @@ describe('createGitHubSlice.fetchPRComments', () => { vi.useRealTimers() } }) - - it('preserves cached checks when the checks IPC fails', async () => { - const store = createTestStore() - const repoPath = '/repo' - const branch = 'feature/test' - const checksCacheKey = `${repoPath}::pr-checks::12` - const cachedChecks = [ - { name: 'build', status: 'completed', conclusion: 'failure', url: null } as const - ] - - store.setState({ - checksCache: { - [checksCacheKey]: { - data: cachedChecks, - fetchedAt: 1, - headSha: 'abc123head' - } - } - } as unknown as Partial) - mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) - - await expect( - store.getState().fetchPRChecks(repoPath, 12, branch, 'abc123head', null, { force: true }) - ).resolves.toEqual(cachedChecks) - - expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(cachedChecks) - expect(store.getState().checksCache[checksCacheKey]?.fetchedAt).toBe(1) - }) - - it('does not return cached checks for a different requested head SHA after IPC failure', async () => { - const store = createTestStore() - const repoPath = '/repo' - const branch = 'feature/test' - const checksCacheKey = `${repoPath}::pr-checks::12` - const oldHeadChecks = [ - { name: 'build', status: 'completed', conclusion: 'success', url: null } as const - ] - - store.setState({ - checksCache: { - [checksCacheKey]: { - data: oldHeadChecks, - fetchedAt: 1, - headSha: 'old-head' - } - } - } as unknown as Partial) - mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) - - await expect( - store.getState().fetchPRChecks(repoPath, 12, branch, 'new-head', null, { force: true }) - ).resolves.toEqual([]) - - expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(oldHeadChecks) - expect(store.getState().checksCache[checksCacheKey]?.headSha).toBe('old-head') - }) }) describe('createGitHubSlice.fetchPRCheckDetails', () => { diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index add36b6cb..7aaf6a21e 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -427,6 +427,7 @@ function bypassesGitHubPRRefreshFreshness(reason: GitHubPRRefreshReason): boolea const CACHE_TTL = 300_000 // 5 minutes (stale data shown instantly, then refreshed) const CHECKS_CACHE_TTL = 60_000 // 1 minute — checks change more frequently +const EMPTY_CHECKS_CACHE_TTL = 10_000 // Why: the NewWorkspace page's work-item list is a browse surface, not a // source of truth, so 60s staleness is fine — stale data renders instantly // while a background refresh keeps it current. @@ -441,7 +442,11 @@ const inflightPRRequests = new Map< { promise: Promise; force: boolean; generation: number; lookupHintKey: string } >() const inflightIssueRequests = new Map>() -const inflightChecksRequests = new Map>() +type InflightChecksRequest = { + promise: Promise + noCache: boolean +} +const inflightChecksRequests = new Map() const inflightCommentsRequests = new Map>() type InflightWorkItems = { promise: Promise @@ -654,6 +659,10 @@ function isFresh(entry: CacheEntry | undefined, ttl = CACHE_TTL): entry is return entry !== undefined && Date.now() - entry.fetchedAt < ttl } +function checksCacheTtl(entry: CacheEntry | undefined): number { + return entry?.data?.length === 0 ? EMPTY_CHECKS_CACHE_TTL : CHECKS_CACHE_TTL +} + function findWorktreeById(state: AppState, worktreeId: string): Worktree | null { for (const worktrees of Object.values(state.worktreesByRepo)) { const worktree = worktrees.find((w) => w.id === worktreeId) @@ -2411,10 +2420,11 @@ export const createGitHubSlice: StateCreator = (s ) : cacheKey const inflightKey = cacheKey + const requestNoCache = options?.noCache === true || options?.force === true const cached = get().checksCache[cacheKey] ?? get().checksCache[legacyCacheKey] if ( - !options?.force && - isFresh(cached, CHECKS_CACHE_TTL) && + !requestNoCache && + isFresh(cached, checksCacheTtl(cached)) && (!headSha || cached.headSha === headSha) ) { const cachedChecks = cached.data ?? [] @@ -2438,7 +2448,16 @@ export const createGitHubSlice: StateCreator = (s const inflightRequest = inflightChecksRequests.get(inflightKey) if (inflightRequest) { - return inflightRequest + if (!requestNoCache || inflightRequest.noCache) { + return inflightRequest.promise + } + // Why: manual refreshes must not inherit an automatic request that may + // still be served from the GitHub CLI cache. Wait, then issue fresh. + await inflightRequest.promise.catch(() => {}) + const latestInflightRequest = inflightChecksRequests.get(inflightKey) + if (latestInflightRequest?.noCache) { + return latestInflightRequest.promise + } } const request = (async () => { @@ -2453,7 +2472,7 @@ export const createGitHubSlice: StateCreator = (s prNumber, headSha, prRepo: prRepo ?? null, - noCache: options?.force + noCache: requestNoCache }, { timeoutMs: 30_000 } ) @@ -2463,7 +2482,7 @@ export const createGitHubSlice: StateCreator = (s prNumber, headSha, prRepo: prRepo ?? null, - noCache: options?.force + noCache: requestNoCache })) as PRCheckDetail[]) set((s) => { const nextState: Partial = { @@ -2505,7 +2524,7 @@ export const createGitHubSlice: StateCreator = (s } })() - inflightChecksRequests.set(inflightKey, request) + inflightChecksRequests.set(inflightKey, { promise: request, noCache: requestNoCache }) return request }, @@ -2924,6 +2943,17 @@ export const createGitHubSlice: StateCreator = (s const checksCacheKeys = [ ...(alias.repoId ? [ + ...(pr.headSha + ? [ + runtimeScopedRepoCacheKey( + alias.repoPath, + alias.repoId, + prChecksCacheSuffix(pr.number, pr.prRepo, pr.headSha), + s.settings, + alias.connectionId + ) + ] + : []), runtimeScopedRepoCacheKey( alias.repoPath, alias.repoId, @@ -2933,6 +2963,17 @@ export const createGitHubSlice: StateCreator = (s ) ] : []), + ...(pr.headSha + ? [ + runtimeScopedRepoCacheKey( + alias.repoPath, + undefined, + prChecksCacheSuffix(pr.number, pr.prRepo, pr.headSha), + s.settings, + alias.connectionId + ) + ] + : []), runtimeScopedRepoCacheKey( alias.repoPath, undefined, @@ -2944,14 +2985,15 @@ export const createGitHubSlice: StateCreator = (s ] const checksEntry = checksCacheKeys .map((key) => s.checksCache[key]) - .find((entry) => entry?.data) - if ( - checksEntry?.data && - checksEntry.headSha && - pr.headSha && - checksEntry.headSha === pr.headSha && - event.outcome.fetchedAt - checksEntry.fetchedAt < CHECKS_CACHE_TTL - ) { + .find( + (entry) => + entry?.data && + entry.headSha && + pr.headSha && + entry.headSha === pr.headSha && + event.outcome.fetchedAt - entry.fetchedAt < checksCacheTtl(entry) + ) + if (checksEntry?.data) { return { ...pr, checksStatus: deriveCheckStatusFromChecks(checksEntry.data) } } return pr