Skip to content

Commit 4aaf7db

Browse files
committed
fix(pr-checks): harden branch PR detection
1 parent d3f41ab commit 4aaf7db

13 files changed

Lines changed: 282 additions & 123 deletions

File tree

electron/ipc/channels.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ export enum IPC {
137137
StartPrChecksWatcher = 'start_pr_checks_watcher',
138138
StopPrChecksWatcher = 'stop_pr_checks_watcher',
139139
DetectPrForBranch = 'detect_pr_for_branch',
140+
RefreshPrChecksWatcher = 'refresh_pr_checks_watcher',
140141
PrChecksUpdate = 'pr_checks_update',
141142

142143
// Logging

electron/ipc/pr-checks.test.ts

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,13 @@ import {
4242
} from './pr-checks.js';
4343

4444
type ExecCb = (err: Error | null, stdout: string, stderr: string) => void;
45-
type GhHandler = (args: string[], cb: ExecCb) => void;
45+
type GhHandler = (args: string[], cb: ExecCb, cmd: string) => void;
4646

4747
function stubGh(handler: GhHandler): string[][] {
4848
const calls: string[][] = [];
49-
const impl = (_cmd: string, args: string[], _opts: unknown, cb: ExecCb) => {
49+
const impl = (cmd: string, args: string[], _opts: unknown, cb: ExecCb) => {
5050
calls.push(args);
51-
handler(args, cb);
51+
handler(args, cb, cmd);
5252
};
5353
vi.mocked(execFile).mockImplementation(impl as unknown as typeof execFile);
5454
return calls;
@@ -206,34 +206,66 @@ describe('detectPrUrlForBranch', () => {
206206
});
207207

208208
it('finds an open PR for a branch', async () => {
209-
const calls = stubGh((_args, cb) =>
210-
cb(null, JSON.stringify([{ url: 'https://github.com/a/b/pull/12' }]), ''),
211-
);
209+
const calls = stubGh((_args, cb, cmd) => {
210+
if (cmd === 'git') {
211+
cb(null, 'abc123\n', '');
212+
return;
213+
}
214+
cb(
215+
null,
216+
JSON.stringify([
217+
{
218+
url: 'https://github.com/a/b/pull/11',
219+
headRefName: 'task/my-branch',
220+
headRefOid: 'old',
221+
},
222+
{
223+
url: 'https://github.com/a/b/pull/12',
224+
headRefName: 'task/my-branch',
225+
headRefOid: 'abc123',
226+
},
227+
]),
228+
'',
229+
);
230+
});
212231
await expect(detectPrUrlForBranch('/repo/worktree', 'task/my-branch')).resolves.toBe(
213232
'https://github.com/a/b/pull/12',
214233
);
215-
expect(calls[0]).toEqual([
234+
expect(calls[0]).toEqual(['rev-parse', 'task/my-branch']);
235+
expect(calls[1]).toEqual([
216236
'pr',
217237
'list',
218238
'--state',
219239
'open',
220240
'--head',
221241
'task/my-branch',
222242
'--json',
223-
'url',
243+
'url,headRefName,headRefOid',
224244
'--limit',
225-
'1',
245+
'20',
226246
]);
227247
});
228248

229249
it('returns null when the branch has no open PR', async () => {
230-
stubGh((_args, cb) => cb(null, JSON.stringify([]), ''));
250+
stubGh((_args, cb, cmd) => cb(null, cmd === 'git' ? 'abc123\n' : JSON.stringify([]), ''));
231251
await expect(detectPrUrlForBranch('/repo/worktree', 'task/no-pr')).resolves.toBe(null);
232252
});
233253

234254
it('rejects malformed PR URLs from gh output', async () => {
235-
stubGh((_args, cb) =>
236-
cb(null, JSON.stringify([{ url: 'https://github.com/a/b/issues/12' }]), ''),
255+
stubGh((_args, cb, cmd) =>
256+
cb(
257+
null,
258+
cmd === 'git'
259+
? 'abc123\n'
260+
: JSON.stringify([
261+
{
262+
url: 'https://github.com/a/b/issues/12',
263+
headRefName: 'task/issue',
264+
headRefOid: 'abc123',
265+
},
266+
]),
267+
'',
268+
),
237269
);
238270
await expect(detectPrUrlForBranch('/repo/worktree', 'task/issue')).resolves.toBe(null);
239271
});

electron/ipc/pr-checks.ts

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ const exec = promisify(execFile);
77

88
const TICK_MS = 30_000;
99
const SETTLED_REFRESH_MS = 5 * 60_000;
10+
const POST_PUSH_REFRESH_GRACE_MS = 2 * 60_000;
1011
const GH_TIMEOUT_MS = 15_000;
1112
const GH_MAX_BUFFER = 4 * 1024 * 1024;
1213

@@ -44,6 +45,8 @@ interface TaskEntry {
4445
lastNotifiedSha: string | null;
4546
/** Outcome at the last notification, so the same (sha, outcome) doesn't refire. */
4647
lastNotifiedOutcome: Exclude<PrChecksOverall, 'pending' | 'none'> | null;
48+
/** After a local push, ignore stale old-SHA check data while GitHub catches up. */
49+
postPushRefreshUntil: number | null;
4750
}
4851

4952
let win: BrowserWindow | null = null;
@@ -114,6 +117,7 @@ export function startPrChecksWatcher(args: {
114117
lastRefreshedAt: 0,
115118
lastNotifiedSha: null,
116119
lastNotifiedOutcome: null,
120+
postPushRefreshUntil: null,
117121
}
118122
: { ...existing, taskName: args.taskName };
119123
tasks.set(args.taskId, next);
@@ -128,6 +132,21 @@ export function stopPrChecksWatcher(taskId: string): void {
128132
if (tasks.size === 0) clearTickInterval();
129133
}
130134

135+
export function refreshPrChecksWatcher(taskId: string): void {
136+
if (disabled) return;
137+
const entry = tasks.get(taskId);
138+
if (!entry) return;
139+
entry.postPushRefreshUntil = Date.now() + POST_PUSH_REFRESH_GRACE_MS;
140+
entry.overall = 'pending';
141+
entry.passing = 0;
142+
entry.pending = Math.max(1, entry.checks.length);
143+
entry.failing = 0;
144+
entry.checks = [];
145+
entry.lastRefreshedAt = Date.now();
146+
sendUpdate(entry);
147+
ensureInterval();
148+
}
149+
131150
/** True if the window currently exists and is visible. */
132151
function windowIsVisible(): boolean {
133152
return !!win && !win.isDestroyed() && win.isVisible();
@@ -209,6 +228,12 @@ async function refreshOne(taskId: string): Promise<void> {
209228
const prevSha = entry.headRefOid;
210229
const shaChanged = prevSha !== null && prevSha !== view.headRefOid;
211230
const firstRefresh = entry.lastRefreshedAt === 0;
231+
const waitingForPostPushHead =
232+
entry.postPushRefreshUntil !== null && !shaChanged && Date.now() < entry.postPushRefreshUntil;
233+
if (waitingForPostPushHead) {
234+
entry.lastRefreshedAt = Date.now();
235+
return;
236+
}
212237
const nothingChanged =
213238
!firstRefresh &&
214239
entry.overall === overall &&
@@ -221,6 +246,9 @@ async function refreshOne(taskId: string): Promise<void> {
221246
if (shaChanged) {
222247
entry.lastNotifiedSha = null;
223248
entry.lastNotifiedOutcome = null;
249+
entry.postPushRefreshUntil = null;
250+
} else if (entry.postPushRefreshUntil !== null && Date.now() >= entry.postPushRefreshUntil) {
251+
entry.postPushRefreshUntil = null;
224252
}
225253

226254
entry.overall = overall;
@@ -356,16 +384,37 @@ export async function detectPrUrlForBranch(
356384
worktreePath: string,
357385
branchName: string,
358386
): Promise<string | null> {
387+
const { stdout: headStdout } = await exec('git', ['rev-parse', branchName], {
388+
cwd: worktreePath,
389+
timeout: GH_TIMEOUT_MS,
390+
maxBuffer: GH_MAX_BUFFER,
391+
});
392+
const localHead = headStdout.trim();
359393
const { stdout } = await exec(
360394
'gh',
361-
['pr', 'list', '--state', 'open', '--head', branchName, '--json', 'url', '--limit', '1'],
395+
[
396+
'pr',
397+
'list',
398+
'--state',
399+
'open',
400+
'--head',
401+
branchName,
402+
'--json',
403+
'url,headRefName,headRefOid',
404+
'--limit',
405+
'20',
406+
],
362407
{ cwd: worktreePath, timeout: GH_TIMEOUT_MS, maxBuffer: GH_MAX_BUFFER },
363408
);
364409
const parsed: unknown = JSON.parse(stdout);
365410
if (!Array.isArray(parsed)) return null;
366-
const first = parsed[0];
367-
if (!first || typeof first !== 'object') return null;
368-
const url = (first as Record<string, unknown>)['url'];
411+
const match = parsed.find((item) => {
412+
if (!item || typeof item !== 'object') return false;
413+
const pr = item as Record<string, unknown>;
414+
return pr['headRefName'] === branchName && pr['headRefOid'] === localHead;
415+
});
416+
if (!match || typeof match !== 'object') return null;
417+
const url = (match as Record<string, unknown>)['url'];
369418
return typeof url === 'string' && isPrUrl(url) ? url : null;
370419
}
371420

electron/ipc/register.ts

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import {
3232
startPrChecksWatcher,
3333
stopPrChecksWatcher,
3434
detectPrUrlForBranch,
35+
refreshPrChecksWatcher,
3536
isPrUrl,
3637
} from './pr-checks.js';
3738
import { readCoverageSummary } from './coverage.js';
@@ -837,15 +838,24 @@ export function registerAllHandlers(win: BrowserWindow): void {
837838
stopPrChecksWatcher(args.taskId);
838839
});
839840
ipcMain.handle(IPC.DetectPrForBranch, (_e, args) => {
840-
assertString(args.worktreePath, 'worktreePath');
841-
assertString(args.branchName, 'branchName');
842-
return detectPrUrlForBranch(args.worktreePath, args.branchName).catch((err: unknown) => {
843-
const code = (err as NodeJS.ErrnoException)?.code;
844-
const stderr = (err as { stderr?: string })?.stderr ?? '';
845-
if (code === 'ENOENT' || /not logged into|authentication required/i.test(stderr)) return null;
846-
console.warn('[pr-checks] branch PR detection failed:', (err as Error)?.message ?? err);
847-
return null;
848-
});
841+
validatePath(args.worktreePath, 'worktreePath');
842+
validateBranchName(args.branchName, 'branchName');
843+
return detectPrUrlForBranch(args.worktreePath, args.branchName)
844+
.then((url) => ({ url }))
845+
.catch((err: unknown) => {
846+
const code = (err as NodeJS.ErrnoException)?.code;
847+
const stderr = (err as { stderr?: string })?.stderr ?? '';
848+
if (code === 'ENOENT') return { url: null, unavailable: 'missing' };
849+
if (/not logged into|authentication required/i.test(stderr)) {
850+
return { url: null, unavailable: 'auth' };
851+
}
852+
console.warn('[pr-checks] branch PR detection failed:', (err as Error)?.message ?? err);
853+
return { url: null };
854+
});
855+
});
856+
ipcMain.handle(IPC.RefreshPrChecksWatcher, (_e, args) => {
857+
assertString(args.taskId, 'taskId');
858+
refreshPrChecksWatcher(args.taskId);
849859
});
850860

851861
// --- Steps content (one-shot read) ---

electron/preload.cjs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ const ALLOWED_CHANNELS = new Set([
124124
'start_pr_checks_watcher',
125125
'stop_pr_checks_watcher',
126126
'detect_pr_for_branch',
127+
'refresh_pr_checks_watcher',
127128
'pr_checks_update',
128129
// Logging
129130
'log_from_renderer',

src/components/TaskBranchInfoBar.tsx

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { revealItemInDir, openInEditor } from '../lib/shell';
55
import { InfoBar } from './InfoBar';
66
import { theme } from '../lib/theme';
77
import { isMac } from '../lib/platform';
8+
import { parseGitHubUrl } from '../lib/github-url';
89
import type { Task } from '../store/types';
910

1011
const infoBarBtnStyle: JSX.CSSProperties = {
@@ -28,6 +29,19 @@ interface TaskBranchInfoBarProps {
2829

2930
export function TaskBranchInfoBar(props: TaskBranchInfoBarProps) {
3031
const mod = isMac ? 'Cmd' : 'Ctrl';
32+
const isPrUrl = (url: string | undefined): boolean => {
33+
const parsed = url ? parseGitHubUrl(url) : null;
34+
return parsed?.type === 'pull' && !!parsed.number;
35+
};
36+
const githubLabel = (url: string): string => url.replace(/^https?:\/\/(www\.)?github\.com\//, '');
37+
const prLinkUrl = () =>
38+
props.task.prUrl ?? (isPrUrl(props.task.githubUrl) ? props.task.githubUrl : undefined);
39+
const sourceLinkUrl = () => {
40+
const prUrl = prLinkUrl();
41+
return props.task.githubUrl && props.task.githubUrl !== prUrl
42+
? props.task.githubUrl
43+
: undefined;
44+
};
3145
const editorTitle = () =>
3246
store.editorCommand
3347
? `Click to open in ${store.editorCommand} · ${mod}+Click to reveal in file manager · ${mod}+Shift+Click to open the project root in ${store.editorCommand}`
@@ -83,7 +97,28 @@ export function TaskBranchInfoBar(props: TaskBranchInfoBarProps) {
8397
</Show>
8498
);
8599
})()}
86-
<Show when={props.task.githubUrl}>
100+
<Show when={sourceLinkUrl()}>
101+
{(url) => (
102+
<button
103+
type="button"
104+
onClick={() => window.open(url(), '_blank')}
105+
title={url()}
106+
style={{ ...infoBarBtnStyle, 'margin-right': '8px', color: theme.accent }}
107+
>
108+
<svg
109+
width="12"
110+
height="12"
111+
viewBox="0 0 16 16"
112+
fill="currentColor"
113+
style={{ 'flex-shrink': '0' }}
114+
>
115+
<path d="M8 0C3.58 0 0 3.58 0 8c0 3.54 2.29 6.53 5.47 7.59.4.07.55-.17.55-.38 0-.19-.01-.82-.01-1.49-2.01.37-2.53-.49-2.69-.94-.09-.23-.48-.94-.82-1.13-.28-.15-.68-.52-.01-.53.63-.01 1.08.58 1.23.82.72 1.21 1.87.87 2.33.66.07-.52.28-.87.51-1.07-1.78-.2-3.64-.89-3.64-3.95 0-.87.31-1.59.82-2.15-.08-.2-.36-1.02.08-2.12 0 0 .67-.21 2.2.82.64-.18 1.32-.27 2-.27.68 0 1.36.09 2 .27 1.53-1.04 2.2-.82 2.2-.82.44 1.1.16 1.92.08 2.12.51.56.82 1.27.82 2.15 0 3.07-1.87 3.75-3.65 3.95.29.25.54.73.54 1.48 0 1.07-.01 1.93-.01 2.2 0 .21.15.46.55.38A8.013 8.013 0 0 0 16 8c0-4.42-3.58-8-8-8Z" />
116+
</svg>
117+
{githubLabel(url())}
118+
</button>
119+
)}
120+
</Show>
121+
<Show when={prLinkUrl()}>
87122
{(url) => {
88123
const pr = () => getPrChecks(props.task.id);
89124
const dotColor = (): string | null => {
@@ -143,7 +178,7 @@ export function TaskBranchInfoBar(props: TaskBranchInfoBarProps) {
143178
>
144179
<path d="M8 0C3.58 0 0 3.58 0 8c0 3.54 2.29 6.53 5.47 7.59.4.07.55-.17.55-.38 0-.19-.01-.82-.01-1.49-2.01.37-2.53-.49-2.69-.94-.09-.23-.48-.94-.82-1.13-.28-.15-.68-.52-.01-.53.63-.01 1.08.58 1.23.82.72 1.21 1.87.87 2.33.66.07-.52.28-.87.51-1.07-1.78-.2-3.64-.89-3.64-3.95 0-.87.31-1.59.82-2.15-.08-.2-.36-1.02.08-2.12 0 0 .67-.21 2.2.82.64-.18 1.32-.27 2-.27.68 0 1.36.09 2 .27 1.53-1.04 2.2-.82 2.2-.82.44 1.1.16 1.92.08 2.12.51.56.82 1.27.82 2.15 0 3.07-1.87 3.75-3.65 3.95.29.25.54.73.54 1.48 0 1.07-.01 1.93-.01 2.2 0 .21.15.46.55.38A8.013 8.013 0 0 0 16 8c0-4.42-3.58-8-8-8Z" />
145180
</svg>
146-
{url().replace(/^https?:\/\/(www\.)?github\.com\//, '')}
181+
{githubLabel(url())}
147182
</button>
148183
);
149184
}}

0 commit comments

Comments
 (0)