From 8e77aca170ea12d31b356e59cefe7f0454f5537e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 5 Apr 2026 11:54:53 -0700 Subject: [PATCH] fix: fetch PR checks by head sha (#312) --- src/main/github/client.test.ts | 70 ++++++++++++++++++- src/main/github/client.ts | 60 +++++++++------- src/main/ipc/github.ts | 12 +++- src/preload/index.d.ts | 2 +- src/preload/index.ts | 2 +- .../components/right-sidebar/ChecksPanel.tsx | 6 +- src/renderer/src/store/slices/github.test.ts | 36 ++++++++-- src/renderer/src/store/slices/github.ts | 5 +- src/shared/types.ts | 4 ++ 9 files changed, 158 insertions(+), 39 deletions(-) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 7ab88b8b655..a3f09515a64 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -12,7 +12,7 @@ vi.mock('util', async () => { } }) -import { getPRForBranch, _resetOwnerRepoCache } from './client' +import { getPRForBranch, getPRChecks, _resetOwnerRepoCache } from './client' describe('getPRForBranch', () => { beforeEach(() => { @@ -241,3 +241,71 @@ describe('getPRForBranch', () => { expect(pr).toBeNull() }) }) + +describe('getPRChecks', () => { + beforeEach(() => { + execFileAsyncMock.mockReset() + _resetOwnerRepoCache() + }) + + it('queries check-runs by PR head SHA when GitHub remote metadata is available', async () => { + execFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + check_runs: [ + { + name: 'build', + status: 'completed', + conclusion: 'success', + html_url: 'https://github.com/acme/widgets/actions/runs/1', + details_url: null + } + ] + }) + }) + + const checks = await getPRChecks('/repo-root', 42, 'head-oid') + + expect(execFileAsyncMock).toHaveBeenNthCalledWith( + 2, + 'gh', + ['api', '--cache', '60s', 'repos/acme/widgets/commits/head-oid/check-runs?per_page=100'], + { cwd: '/repo-root', encoding: 'utf-8' } + ) + expect(checks).toEqual([ + { + name: 'build', + status: 'completed', + conclusion: 'success', + url: 'https://github.com/acme/widgets/actions/runs/1' + } + ]) + }) + + it('falls back to gh pr checks when the cached head SHA no longer resolves', async () => { + execFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) + .mockRejectedValueOnce(new Error('gh: No commit found for SHA: stale-head (HTTP 422)')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ name: 'lint', state: 'PASS', link: 'https://example.com/lint' }]) + }) + + const checks = await getPRChecks('/repo-root', 42, 'stale-head') + + expect(execFileAsyncMock).toHaveBeenNthCalledWith( + 3, + 'gh', + ['pr', 'checks', '42', '--json', 'name,state,link'], + { cwd: '/repo-root', encoding: 'utf-8' } + ) + expect(checks).toEqual([ + { + name: 'lint', + status: 'completed', + conclusion: 'success', + url: 'https://example.com/lint' + } + ]) + }) +}) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 148c5d71ad3..baff4c49c64 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -103,6 +103,7 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< checksStatus: deriveCheckStatus(data.statusCheckRollup), updatedAt: data.updatedAt, mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', + headSha: data.headRefOid, conflictSummary } } catch { @@ -120,40 +121,47 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< export async function getPRChecks( repoPath: string, prNumber: number, - branch?: string, + headSha?: string, options?: { noCache?: boolean } ): Promise { - const ownerRepo = branch ? await getOwnerRepo(repoPath) : null + const ownerRepo = headSha ? await getOwnerRepo(repoPath) : null await acquire() try { - if (ownerRepo && branch) { + if (ownerRepo && headSha) { // Why: --cache 60s saves rate-limit budget during polling, but when the // user explicitly clicks refresh we must skip it so gh fetches fresh data. const cacheArgs = options?.noCache ? [] : ['--cache', '60s'] - const { stdout } = await execFileAsync( - 'gh', - [ - 'api', - ...cacheArgs, - `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodeURIComponent(branch)}/check-runs?per_page=100` - ], - { cwd: repoPath, encoding: 'utf-8' } - ) - const data = JSON.parse(stdout) as { - check_runs: { - name: string - status: string - conclusion: string | null - html_url: string - details_url: string | null - }[] + try { + const { stdout } = await execFileAsync( + 'gh', + [ + 'api', + ...cacheArgs, + `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodeURIComponent(headSha)}/check-runs?per_page=100` + ], + { cwd: repoPath, encoding: 'utf-8' } + ) + const data = JSON.parse(stdout) as { + check_runs: { + name: string + status: string + conclusion: string | null + html_url: string + details_url: string | null + }[] + } + return data.check_runs.map((d) => ({ + name: d.name, + status: mapCheckRunRESTStatus(d.status), + conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion), + url: d.details_url || d.html_url || null + })) + } catch (err) { + // Why: a PR can outlive the cached head SHA after force-pushes or remote + // rewrites. Falling back to `gh pr checks` keeps the panel populated + // instead of rendering a false "no checks" state from a stale commit. + console.warn('getPRChecks via head SHA failed, falling back to gh pr checks:', err) } - return data.check_runs.map((d) => ({ - name: d.name, - status: mapCheckRunRESTStatus(d.status), - conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion), - url: d.details_url || d.html_url || null - })) } // Fallback: no branch provided or non-GitHub remote const { stdout } = await execFileAsync( diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index 40442f36de2..ed650f75f39 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -37,9 +37,17 @@ export function registerGitHubHandlers(store: Store): void { ipcMain.handle( 'gh:prChecks', - (_event, args: { repoPath: string; prNumber: number; branch?: string; noCache?: boolean }) => { + ( + _event, + args: { + repoPath: string + prNumber: number + headSha?: string + noCache?: boolean + } + ) => { const repoPath = assertRegisteredRepoPath(args.repoPath, store) - return getPRChecks(repoPath, args.prNumber, args.branch, { + return getPRChecks(repoPath, args.prNumber, args.headSha, { noCache: args.noCache }) } diff --git a/src/preload/index.d.ts b/src/preload/index.d.ts index b3cdc0e64c3..ba5aeae567f 100644 --- a/src/preload/index.d.ts +++ b/src/preload/index.d.ts @@ -70,7 +70,7 @@ type GhApi = { prChecks: (args: { repoPath: string prNumber: number - branch?: string + headSha?: string noCache?: boolean }) => Promise updatePRTitle: (args: { repoPath: string; prNumber: number; title: string }) => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 3d6bdf6e96b..74700ddb55a 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -189,7 +189,7 @@ const api = { prChecks: (args: { repoPath: string prNumber: number - branch?: string + headSha?: string noCache?: boolean }): Promise => ipcRenderer.invoke('gh:prChecks', args), diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 1fa9a2dd7d8..e132fec347c 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -96,7 +96,9 @@ export default function ChecksPanel(): React.JSX.Element { } setChecksLoading(true) try { - const result = await fetchPRChecks(repo.path, targetPRNumber, branch, { force }) + const result = await fetchPRChecks(repo.path, targetPRNumber, branch, pr?.headSha, { + force + }) setChecks(result) // Exponential backoff: if checks haven't changed, double the interval (cap 120s). @@ -114,7 +116,7 @@ export default function ChecksPanel(): React.JSX.Element { setChecksLoading(false) } }, - [repo, prNumber, branch, fetchPRChecks] + [repo, prNumber, branch, pr?.headSha, fetchPRChecks] ) // Fetch checks on mount + poll with exponential backoff diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 35d06f99363..f9ca0d0469d 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -37,6 +37,7 @@ function makePR(overrides: Partial = {}): PRInfo { checksStatus: 'pending', updatedAt: '2026-03-28T00:00:00Z', mergeable: 'UNKNOWN', + headSha: 'head-oid', ...overrides } } @@ -71,7 +72,7 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'lint', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, { force: true }) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success') }) @@ -96,7 +97,7 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'integration', status: 'completed', conclusion: 'failure', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, { force: true }) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('failure') }) @@ -120,7 +121,9 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'build', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, { force: true }) + await store + .getState() + .fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, undefined, { force: true }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success') }) @@ -146,7 +149,7 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'build', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, { force: true }) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true }) await vi.advanceTimersByTimeAsync(1000) expect(mockApi.cache.setGitHub).toHaveBeenCalledWith({ @@ -193,6 +196,31 @@ describe('createGitHubSlice.fetchPRChecks', () => { } }) }) + + it('passes the cached PR head SHA to the checks IPC request', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const prCacheKey = `${repoPath}::${branch}` + + store.setState({ + prCache: { + [prCacheKey]: { + data: makePR({ headSha: 'abc123head' }), + fetchedAt: 1 + } + } + }) + + await store.getState().fetchPRChecks(repoPath, 12, branch, 'abc123head', { force: true }) + + expect(mockApi.gh.prChecks).toHaveBeenCalledWith({ + repoPath, + prNumber: 12, + headSha: 'abc123head', + noCache: true + }) + }) }) describe('createGitHubSlice.fetchPRForBranch', () => { diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 82b1e76a684..45507e8bdbf 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -58,6 +58,7 @@ export type GitHubSlice = { repoPath: string, prNumber: number, branch?: string, + headSha?: string, options?: FetchOptions ) => Promise initGitHubCache: () => Promise @@ -170,7 +171,7 @@ export const createGitHubSlice: StateCreator = (s return request }, - fetchPRChecks: async (repoPath, prNumber, branch, options): Promise => { + fetchPRChecks: async (repoPath, prNumber, branch, headSha, options): Promise => { const cacheKey = `${repoPath}::pr-checks::${prNumber}` const cached = get().checksCache[cacheKey] if (!options?.force && isFresh(cached, CHECKS_CACHE_TTL)) { @@ -193,7 +194,7 @@ export const createGitHubSlice: StateCreator = (s const checks = (await window.api.gh.prChecks({ repoPath, prNumber, - branch, + headSha, noCache: options?.force })) as PRCheckDetail[] set((s) => { diff --git a/src/shared/types.ts b/src/shared/types.ts index 5e01d70b059..0f1f5c41601 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -120,6 +120,10 @@ export type PRInfo = { checksStatus: CheckStatus updatedAt: string mergeable: PRMergeableState + // Why: check-runs are keyed by the PR head commit, not the mutable branch name. + // Keeping the head SHA in cached PR metadata lets the checks panel poll the + // correct commit without re-querying GitHub or guessing from local branch refs. + headSha?: string conflictSummary?: PRConflictSummary }