Skip to content

Feat/instant pr detection simplified#1305

Open
aryanpatel-ctrl wants to merge 4 commits intogeneralaction:mainfrom
aryanpatel-ctrl:feat/instant-pr-detection-simplified
Open

Feat/instant pr detection simplified#1305
aryanpatel-ctrl wants to merge 4 commits intogeneralaction:mainfrom
aryanpatel-ctrl:feat/instant-pr-detection-simplified

Conversation

@aryanpatel-ctrl
Copy link
Contributor

Summary

Instant PR detection when created via CLI (e.g., gh pr create) instead of waiting up to 30s for the next poll cycle.

Simplified resubmission addressing feedback from #1284.

Changes :->

  • Watch terminal output for GitHub PR URL patterns

  • Trigger immediate refreshPrStatus() when detected

  • Skip SSH PTYs where cwd is undefined to avoid wrong task refresh

  • Implementation :->
    Simple detection per maintainer's suggestion - no tail buffer or cooldown complexity needed since refreshPrStatus already deduplicates.

  • Test plan :->

  • Run gh pr create in agent terminal

  • Verify changes panel updates immediately with PR status

  • Verify SSH terminals don't cause wrong task refresh

B100EFA7-B3A6-4C77-99B0-FD33F75B419F_1_206_a

@vercel
Copy link

vercel bot commented Mar 5, 2026

@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-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR adds event-driven PR status refresh by scanning PTY terminal output for GitHub PR URL patterns and triggering an immediate refreshPrStatus call when one is detected, replacing the previous 30s poll-only approach. The implementation is clean and minimal, adding a module-level regex, a small emission helper, a new IPC channel wired through contextBridge, and a useEffect on the renderer side.

Key points:

  • SSH PTY overhead: maybeEmitPrUrlDetected is registered in the onData handler for SSH PTYs as well as local PTYs. Since SSH PtyRecord entries have no cwd, the renderer guard (if (!event?.cwd) return) correctly prevents wrong-task refreshes — but the regex still runs on every SSH PTY data chunk and an IPC message is sent whenever an SSH terminal happens to print a GitHub URL. Moving the cwd guard into maybeEmitPrUrlDetected (before the regex) would skip the work entirely.
  • Stale spawn-directory cwd: The cwd stored in ptys is the directory the PTY was spawned in, not the shell's current directory. If a user cds to a different repository inside the same terminal and runs gh pr create, refreshPrStatus is fired for the original spawn-directory task rather than the actual repository.
  • No per-PTY cooldown: refreshPrStatus deduplicates concurrent in-flight requests but not sequential ones. Terminal output that repeatedly contains GitHub PR URLs (e.g., gh pr view, logs, test output) can trigger multiple refresh calls back-to-back with no rate limiting.
  • AGENTS.md had its trailing newline removed (cosmetic).

Confidence Score: 3/5

  • Safe to merge with minor issues — the stale spawn-directory concern and missing SSH early-exit are worth addressing before landing
  • The core feature works correctly for the primary use case (local PTY, user doesn't change directories). The SSH guard in the renderer prevents wrong-task refreshes for SSH terminals. However, the regex runs unnecessarily on SSH PTY chunks, the stored cwd can silently point to the wrong task if the user navigates inside the terminal, and repeated PR URL output can fire sequential refreshes without rate limiting. None of these are blocking bugs but they are correctness and efficiency concerns.
  • src/main/services/ptyIpc.ts (SSH PTY overhead + missing cooldown) and src/renderer/hooks/useAutoPrRefresh.ts (stale-cwd task mismatch)

Important Files Changed

Filename Overview
src/main/services/ptyIpc.ts Adds maybeEmitPrUrlDetected hooked into all PTY onData handlers including SSH PTYs; SSH paths run the regex unnecessarily and emit IPC messages that are silently discarded by the renderer.
src/renderer/hooks/useAutoPrRefresh.ts New effect subscribes to PTY PR URL events and calls refreshPrStatus(event.cwd), but cwd is the spawn-time directory — may target the wrong task if the user cds inside the terminal before running gh pr create.
src/main/services/ptyManager.ts Adds cwd field to local PTY records at spawn time and exports getPtyCwd; SSH PTY records intentionally omit cwd, which is correctly relied upon by the renderer guard.
src/main/preload.ts Exposes onPtyPrUrlDetected via contextBridge following the same pattern as existing IPC listeners; looks correct.
src/renderer/types/electron-api.d.ts Type declarations added for onPtyPrUrlDetected in both global and ElectronAPI interface; consistent with the implementation.
AGENTS.md Minor: trailing newline removed from the last line of the file.

Sequence Diagram

sequenceDiagram
    participant Terminal as Terminal (PTY)
    participant ptyIpc as ptyIpc.ts (main)
    participant ptyManager as ptyManager.ts (main)
    participant Preload as preload.ts (contextBridge)
    participant Hook as useAutoPrRefresh.ts (renderer)
    participant Store as prStatusStore.ts (renderer)

    Terminal->>ptyIpc: onData(chunk)
    ptyIpc->>ptyIpc: maybeEmitPrUrlDetected(id, chunk)
    ptyIpc->>ptyIpc: PR_URL_RE.match(chunk)
    alt URL match found
        ptyIpc->>ptyManager: getPtyCwd(id)
        ptyManager-->>ptyIpc: cwd (undefined for SSH PTYs)
        ptyIpc->>Preload: safeSendToOwner → pty:pr-url-detected { id, url, cwd }
        Preload->>Hook: onPtyPrUrlDetected listener fires
        alt cwd is defined (local PTY)
            Hook->>Store: refreshPrStatus(event.cwd)
            Store->>Store: deduplicate via pending map
            Store-->>Hook: PrStatus updated
        else cwd is undefined (SSH PTY)
            Hook->>Hook: early return (no refresh)
        end
    end
Loading

Last reviewed commit: c78ae73

  - Add early return when cwd is undefined to skip SSH PTYs
  - Add 5-second cooldown per PTY to prevent repeated refreshes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant