feat: instant PR detection when created via CLI#1284
feat: instant PR detection when created via
CLI#1284aryanpatel-ctrl wants to merge 1 commit intogeneralaction:mainfrom
Conversation
|
@aryanpatel-ctrl is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds instant PR detection for the CLI workflow: PTY output is scanned for GitHub PR URLs using a regex, and when one is found, a Key changes and concerns:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/main/services/ptyIpc.ts | Adds maybeEmitPrUrlDetected which scans PTY output for GitHub PR URLs and emits pty:pr-url-detected events; the shared global regex with g flag is safe with .match() today but fragile, and the single-entry cooldown map incorrectly deduplicates when multiple URLs arrive in the same chunk. |
| src/main/services/ptyManager.ts | Adds cwd to the local PTY record on startPty and exports getPtyCwd(); SSH PTYs still store no cwd, so getPtyCwd always returns undefined for them — this is the root cause of the wrong-task refresh bug in the renderer. |
| src/renderer/hooks/useAutoPrRefresh.ts | New effect subscribes to onPtyPrUrlDetected and calls refreshPrStatus; the `event.cwd |
| src/main/preload.ts | Exposes onPtyPrUrlDetected through the context bridge; implementation mirrors the existing listener pattern with correct cleanup via removeListener. |
| src/test/main/ptyIpc.test.ts | Upgrades MockProc to capture onData handlers via emitData, mocks getPtyCwd, and adds a test for basic PR URL detection; no tests for cooldown behaviour, multi-URL chunks, or split-chunk detection. |
Sequence Diagram
sequenceDiagram
participant Shell as PTY Shell Process
participant PtyIpc as ptyIpc (main)
participant PtyMgr as ptyManager (main)
participant Preload as preload.ts
participant Hook as useAutoPrRefresh (renderer)
participant Store as prStatusStore
Shell->>PtyIpc: onData(chunk)
PtyIpc->>PtyIpc: maybeEmitPrUrlDetected(id, chunk)
PtyIpc->>PtyIpc: concat priorTail + chunk, run PR_URL_PATTERN.match()
alt URL found & not in cooldown
PtyIpc->>PtyMgr: getPtyCwd(id)
PtyMgr-->>PtyIpc: cwd (undefined for SSH PTYs)
PtyIpc->>Preload: safeSendToOwner → pty:pr-url-detected {id, url, cwd}
Preload->>Hook: onPtyPrUrlDetected listener({id, url, cwd})
Hook->>Hook: targetPath = cwd || activeTaskPath
Hook->>Store: refreshPrStatus(targetPath)
else URL in cooldown or not matched
PtyIpc-->>Shell: (no event)
end
Last reviewed commit: 8157ced
| for (const url of uniqueMatches) { | ||
| const last = ptyLastDetectedPr.get(id); | ||
| if (last && last.url === url && now - last.at < PR_URL_DETECTION_COOLDOWN_MS) { | ||
| continue; | ||
| } | ||
| ptyLastDetectedPr.set(id, { url, at: now }); | ||
| safeSendToOwner(id, 'pty:pr-url-detected', { id, url, cwd }); | ||
| } |
There was a problem hiding this comment.
Single-entry cooldown map breaks dedup for multiple URLs
ptyLastDetectedPr stores only one {url, at} entry per PTY ID. When the loop iterates over uniqueMatches containing more than one URL, each iteration overwrites the previous entry. This means on the very next invocation the cooldown comparison (last.url === url) will only match the last URL written, so every earlier URL in the set bypasses the cooldown and fires again.
Example scenario with two PRs in the same chunk:
- URL-A emitted →
map[id] = {url: URL-A, at: now} - URL-B emitted →
map[id] = {url: URL-B, at: now}← overwrites URL-A - Next chunk (within 1500 ms) still contains URL-A in priorTail →
last.url === URL-B !== URL-A→ cooldown skipped → URL-A fires again
Change ptyLastDetectedPr to track a separate timestamp per URL:
const ptyLastDetectedPr = new Map<string, Map<string, number>>();
// inside maybeEmitPrUrlDetected:
let urlMap = ptyLastDetectedPr.get(id);
if (!urlMap) {
urlMap = new Map();
ptyLastDetectedPr.set(id, urlMap);
}
for (const url of uniqueMatches) {
const lastAt = urlMap.get(url) ?? 0;
if (now - lastAt < PR_URL_DETECTION_COOLDOWN_MS) continue;
urlMap.set(url, now);
safeSendToOwner(id, 'pty:pr-url-detected', { id, url, cwd });
}
Also update clearPtyData to delete the nested map entry correctly.
| function maybeEmitPrUrlDetected(id: string, chunk: string): void { | ||
| if (!chunk) return; | ||
|
|
||
| const priorTail = ptyPrDetectionTails.get(id) || ''; | ||
| const searchable = `${priorTail}${chunk}`; | ||
| ptyPrDetectionTails.set( | ||
| id, | ||
| searchable.length > PR_URL_DETECTION_TAIL_CHARS | ||
| ? searchable.slice(-PR_URL_DETECTION_TAIL_CHARS) | ||
| : searchable | ||
| ); | ||
|
|
||
| const matches = searchable.match(PR_URL_PATTERN); | ||
| if (!matches || matches.length === 0) return; | ||
|
|
||
| const uniqueMatches = Array.from(new Set(matches)); | ||
| const now = Date.now(); | ||
| const cwd = getPtyCwd(id); | ||
|
|
||
| for (const url of uniqueMatches) { | ||
| const last = ptyLastDetectedPr.get(id); | ||
| if (last && last.url === url && now - last.at < PR_URL_DETECTION_COOLDOWN_MS) { | ||
| continue; | ||
| } | ||
| ptyLastDetectedPr.set(id, { url, at: now }); | ||
| safeSendToOwner(id, 'pty:pr-url-detected', { id, url, cwd }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Tail buffer causes spurious re-detection after cooldown window
searchable is always priorTail + chunk, so a URL that was detected and stored in the tail will be re-matched on every subsequent chunk. Once the 1500 ms cooldown expires, the next arriving byte of terminal output (shell prompt, cursor blink, etc.) will re-emit the event and trigger an unnecessary refreshPrStatus call. This cycle repeats every 1500 ms for as long as the URL remains within the last 512 characters of output.
The tail mechanism exists to correctly detect URLs split across chunk boundaries. Once a full URL has been matched, the portion of the tail that contains the already-matched URL should no longer participate in future searches. Consider tracking the byte offset up to which detection has already run:
// After emitting at least one URL, advance the tail to start *after* the match
// so re-matching the same text is avoided.
ptyPrDetectionTails.set(id, chunk.slice(-PR_URL_DETECTION_TAIL_CHARS));Alternatively, clear the tail after a successful match and accept the small risk of a cross-chunk split (extremely rare for a 50-char URL across a 16 ms flush window).
| useEffect(() => { | ||
| const unsubscribe = window.electronAPI.onPtyPrUrlDetected?.((event) => { | ||
| const targetPath = event?.cwd || activeTaskPath; | ||
| if (!targetPath) return; | ||
| const now = Date.now(); | ||
| const last = lastPtyPrRefresh.current[targetPath] ?? 0; | ||
| if (now - last < PTY_PR_EVENT_COOLDOWN_MS) return; | ||
| lastPtyPrRefresh.current[targetPath] = now; | ||
| refreshPrStatus(targetPath).catch(() => {}); | ||
| }); | ||
|
|
||
| return () => { | ||
| unsubscribe?.(); | ||
| }; | ||
| }, [activeTaskPath]); |
There was a problem hiding this comment.
lastPtyPrRefresh ref object grows without bound
lastPtyPrRefresh.current is a plain Record<string, number> that accumulates one entry per unique targetPath ever seen. Entries are written but never pruned, so over a long-running session with many tasks this map will grow indefinitely.
Consider capping the number of tracked paths or deleting entries on cleanup:
| useEffect(() => { | |
| const unsubscribe = window.electronAPI.onPtyPrUrlDetected?.((event) => { | |
| const targetPath = event?.cwd || activeTaskPath; | |
| if (!targetPath) return; | |
| const now = Date.now(); | |
| const last = lastPtyPrRefresh.current[targetPath] ?? 0; | |
| if (now - last < PTY_PR_EVENT_COOLDOWN_MS) return; | |
| lastPtyPrRefresh.current[targetPath] = now; | |
| refreshPrStatus(targetPath).catch(() => {}); | |
| }); | |
| return () => { | |
| unsubscribe?.(); | |
| }; | |
| }, [activeTaskPath]); | |
| return () => { | |
| unsubscribe?.(); | |
| lastPtyPrRefresh.current = {}; | |
| }; |
This is safe because the cooldown's purpose is to de-bounce rapid duplicate events from the same session; resetting on re-subscription (i.e., when activeTaskPath changes) loses at most one suppressed event, which is acceptable.
Additional Comments (1)
In If PR detection for SSH is intentionally out of scope, the |
|
@greptileai review |
| // Event-driven refresh when terminal output includes a GitHub PR URL. | ||
| useEffect(() => { | ||
| const unsubscribe = window.electronAPI.onPtyPrUrlDetected?.((event) => { | ||
| const targetPath = event?.cwd || activeTaskPath; |
There was a problem hiding this comment.
SSH PTY events fall back to wrong task path
event.cwd is always undefined for SSH PTYs because startSshPty stores the PTY record without a cwd field: ptys.set(id, { id, proc, kind: 'ssh', cols, rows }). When a user runs gh pr create inside an SSH terminal that belongs to a background task, targetPath falls back to activeTaskPath (the currently-viewed task) and refreshPrStatus fires for the wrong project entirely.
A simple guard on event.cwd prevents the false-positive refresh for SSH terminals:
| const targetPath = event?.cwd || activeTaskPath; | |
| const targetPath = event?.cwd; | |
| if (!targetPath) return; |
If SSH support matters, the PTY record's kind should also be threaded through the event payload so the renderer can decide whether to fall back.
| : searchable | ||
| ); | ||
|
|
||
| const matches = searchable.match(PR_URL_PATTERN); |
There was a problem hiding this comment.
Shared global regex lastIndex risk
PR_URL_PATTERN is declared at module scope with the g flag. String.prototype.match() with a global regex correctly resets lastIndex before and after each call, so the current usage with .match() is safe. However, if any future code path calls PR_URL_PATTERN.exec() or PR_URL_PATTERN.test() without resetting lastIndex first, the stateful regex object will silently skip matches. Using a factory or local regex literal is more defensive:
| const matches = searchable.match(PR_URL_PATTERN); | |
| const matches = searchable.match( | |
| /https?:\/\/github\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/pull\/\d+\b/gi | |
| ); |
Alternatively, declare the constant without the g flag and use a local copy inside the function.
| for (const url of uniqueMatches) { | ||
| const last = ptyLastDetectedPr.get(id); | ||
| if (last && last.url === url && now - last.at < PR_URL_DETECTION_COOLDOWN_MS) { | ||
| continue; | ||
| } | ||
| ptyLastDetectedPr.set(id, { url, at: now }); |
There was a problem hiding this comment.
ptyLastDetectedPr only tracks one URL per PTY
ptyLastDetectedPr maps a PTY ID to a single { url, at } pair. Inside the loop over uniqueMatches, each iteration overwrites the previous entry. When two URLs appear in the same chunk, the first URL's cooldown record is immediately overwritten by the second, so on the next chunk the first URL is no longer protected by the cooldown and will be re-emitted.
Consider tracking a timestamp per URL rather than per PTY:
const ptyLastDetectedPr = new Map<string, Map<string, number>>();
// inside the loop:
let urlMap = ptyLastDetectedPr.get(id);
if (!urlMap) {
urlMap = new Map();
ptyLastDetectedPr.set(id, urlMap);
}
const lastAt = urlMap.get(url) ?? 0;
if (now - lastAt < PR_URL_DETECTION_COOLDOWN_MS) continue;
urlMap.set(url, now);
safeSendToOwner(id, 'pty:pr-url-detected', { id, url, cwd });The corresponding cleanup in clearPtyData would then call ptyLastDetectedPr.delete(id) as before.
|
Hey, thanks for working on this! The instant detection is a great idea - waiting up to 30s after gh pr create is a real pain point. A couple of things to address:
URL splitting across chunks is practically impossible with the 16ms flush interval, and the 30s poll is the backstop anyway.
These changes would bring the diff down to ~30-40 lines while keeping the full feature. Happy to help if you have any questions! Closing this for now — please feel free to reopen with the suggested changes, happy to review again! |
Summary
Changes
Test plan