Optimize PR refresh queue usage

Co-authored-by: Orca <help@stably.ai>
2026-10-13 08:02:52 +00:00 · 2026-05-14 02:50:03 -04:00
parent 4aca41adfe
commit 93bf77f271
7 changed files with 315 additions and 73 deletions
@@ -1159,66 +1159,72 @@ export async function getPRChecks(
  const ownerRepo = headSha ? await getOwnerRepo(repoPath) : null
  if (ownerRepo && headSha) {
    await assertRateLimitBudget('core')
-  }
-  // Why: even the REST check-runs path can fall back to `gh pr checks`, so
-  // guard GraphQL before acquiring the global gh lock.
-  await assertRateLimitBudget('graphql')
-  await acquire()
-  try {
-    if (ownerRepo && headSha) {
+    await acquire()
+    try {
      // 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']
-      try {
-        const { stdout } = await ghExecFileAsync(
-          [
-            'api',
-            ...cacheArgs,
-            `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodeURIComponent(headSha)}/check-runs?per_page=100`
-          ],
-          { cwd: repoPath }
-        )
-        noteRateLimitSpend('core')
-        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)
+      const { stdout } = await ghExecFileAsync(
+        [
+          'api',
+          ...cacheArgs,
+          `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodeURIComponent(headSha)}/check-runs?per_page=100`
+        ],
+        { cwd: repoPath }
+      )
+      noteRateLimitSpend('core')
+      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)
+    } finally {
+      release()
+    }
+  }
+
+  // Fallback: no branch provided or non-GitHub remote
+  // Why: the REST check-runs path spends core only. Guard GraphQL only when
+  // we actually fall back to `gh pr checks`, so a low GraphQL bucket does not
+  // block fresh REST check data. Keep this outside the gh lock because the
+  // guard may need its own `gh api rate_limit` call.
+  try {
+    await assertRateLimitBudget('graphql')
+    await acquire()
+    try {
+      const { stdout } = await ghExecFileAsync(
+        ['pr', 'checks', String(prNumber), '--json', 'name,state,link'],
+        { cwd: repoPath }
+      )
+      noteRateLimitSpend('graphql')
+      const data = JSON.parse(stdout) as { name: string; state: string; link: string }[]
+      return data.map((d) => ({
+        name: d.name,
+        status: mapCheckStatus(d.state),
+        conclusion: mapCheckConclusion(d.state),
+        url: d.link || null
+      }))
+    } finally {
+      release()
    }
-    // Fallback: no branch provided or non-GitHub remote
-    const { stdout } = await ghExecFileAsync(
-      ['pr', 'checks', String(prNumber), '--json', 'name,state,link'],
-      { cwd: repoPath }
-    )
-    noteRateLimitSpend('graphql')
-    const data = JSON.parse(stdout) as { name: string; state: string; link: string }[]
-    return data.map((d) => ({
-      name: d.name,
-      status: mapCheckStatus(d.state),
-      conclusion: mapCheckConclusion(d.state),
-      url: d.link || null
-    }))
  } catch (err) {
    console.warn('getPRChecks failed:', err)
    return []
-  } finally {
-    release()
  }
 }

@@ -547,7 +547,7 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise
      message: `Cannot refresh PR for this worktree: ${skippedReason}`,
      fetchedAt: Date.now()
    }
-    broadcast({ aliases, reason: 'manual', status: 'skipped', skippedReason })
+    broadcast({ aliases: [alias], reason: 'manual', status: 'skipped', skippedReason })
    return outcome
  }

@@ -166,7 +166,13 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi
    'gh:refreshPRNow',
    async (_event, args: { candidate: GitHubPRRefreshCandidate }) => {
      const repo = assertRegisteredRepo(args.candidate.repoPath, store)
-      const outcome = await refreshPRNow({ ...args.candidate, repoPath: repo.path })
+      const outcome = await refreshPRNow({
+        ...args.candidate,
+        repoPath: repo.path,
+        repoId: repo.id,
+        connectionId: repo.connectionId ?? args.candidate.connectionId,
+        connectionState: repo.connectionId ? 'connected' : args.candidate.connectionState
+      })
      recordPRIfNeeded(repo, outcome)
      return outcome
    }
@@ -184,7 +190,13 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi
    ) => {
      const repo = assertRegisteredRepo(args.candidate.repoPath, store)
      enqueuePRRefresh(
-        { ...args.candidate, repoPath: repo.path, repoId: repo.id },
+        {
+          ...args.candidate,
+          repoPath: repo.path,
+          repoId: repo.id,
+          connectionId: repo.connectionId ?? args.candidate.connectionId,
+          connectionState: repo.connectionId ? 'connected' : args.candidate.connectionState
+        },
        args.reason,
        args.priority ?? 0
      )
@@ -197,7 +209,13 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi
    (event, args: { candidates: GitHubPRRefreshCandidate[]; generation: number }) => {
      const candidates = args.candidates.map((candidate) => {
        const repo = assertRegisteredRepo(candidate.repoPath, store)
-        return { ...candidate, repoPath: repo.path, repoId: repo.id }
+        return {
+          ...candidate,
+          repoPath: repo.path,
+          repoId: repo.id,
+          connectionId: repo.connectionId ?? candidate.connectionId,
+          connectionState: repo.connectionId ? 'connected' : candidate.connectionState
+        }
      })
      reportVisiblePRRefreshCandidates(candidates, args.generation, event.sender.id)
      return true
@@ -77,6 +77,7 @@ export default function ChecksPanel(): React.JSX.Element {
  // Find active worktree and repo
  const branch = activeWorktree ? activeWorktree.branch.replace(/^refs\/heads\//, '') : ''
  const isFolder = repo ? isFolderRepo(repo) : false
+  const isRemoteRepo = Boolean(repo?.connectionId)
  const prCacheKey = repo && branch ? `${repo.path}::${branch}` : ''
  const refreshContextKey = `${activeWorktreeId ?? ''}::${repo?.path ?? ''}::${branch}`
  if (refreshContextKey !== refreshContextKeyRef.current) {
@@ -549,24 +550,28 @@ export default function ChecksPanel(): React.JSX.Element {
        <div className="text-sm font-medium text-foreground">
          {operationInProgress
            ? `${operationLabel} in progress`
-            : isQueuedPRRefresh
-              ? 'Checking for pull request'
-              : isInFlightPRRefresh
+            : isRemoteRepo
+              ? 'Checks unavailable'
+              : isQueuedPRRefresh
                ? 'Checking for pull request'
-                : 'No pull request found'}
+                : isInFlightPRRefresh
+                  ? 'Checking for pull request'
+                  : 'No pull request found'}
        </div>
        <div className="mt-1 text-xs text-muted-foreground">
          {operationInProgress
            ? 'PR checks will be available after the operation completes'
-            : isQueuedPRRefresh
-              ? 'Waiting to refresh GitHub status for this branch'
-              : isInFlightPRRefresh
-                ? 'Refreshing GitHub status for this branch'
-                : isPausedPRRefresh
-                  ? 'GitHub refresh is paused by the current rate-limit budget'
-                  : 'Push your branch and open a PR to see checks here'}
+            : isRemoteRepo
+              ? 'PR refresh is not supported for remote worktrees yet'
+              : isQueuedPRRefresh
+                ? 'Waiting to refresh GitHub status for this branch'
+                : isInFlightPRRefresh
+                  ? 'Refreshing GitHub status for this branch'
+                  : isPausedPRRefresh
+                    ? 'GitHub refresh is paused by the current rate-limit budget'
+                    : 'Push your branch and open a PR to see checks here'}
        </div>
-        {!operationInProgress && (
+        {!operationInProgress && !isRemoteRepo && (
          <button
            className="mt-3 px-3 py-1 text-xs font-medium rounded-md border border-border bg-accent/50 text-foreground hover:bg-accent transition-colors disabled:opacity-50"
            disabled={emptyRefreshing}
@@ -391,6 +391,10 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp

  const virtualItems = virtualizer.getVirtualItems()
  useEffect(() => {
+    if (document.visibilityState !== 'visible') {
+      lastVisibleRefreshKeyRef.current = '__document_hidden__'
+      return
+    }
    if (groupBy !== 'pr-status' && !cardProps.includes('pr') && !cardProps.includes('ci')) {
      if (lastVisibleRefreshKeyRef.current !== '__hidden__') {
        lastVisibleRefreshKeyRef.current = '__hidden__'
@@ -261,6 +261,36 @@ describe('createGitHubSlice.fetchPRForBranch', () => {

    expect(store.getState().prCache[prCacheKey]?.data).toMatchObject({ number: 99 })
  })
+
+  it('does not call GitHub refresh IPC for SSH-backed repos', async () => {
+    const store = createTestStore()
+    const repoPath = '/repo'
+    const branch = 'feature/test'
+
+    store.setState({
+      repos: [
+        {
+          id: 'repo-1',
+          path: repoPath,
+          name: 'repo',
+          kind: 'git',
+          connectionId: 'ssh-1'
+        }
+      ],
+      prCache: {
+        [`${repoPath}::${branch}`]: {
+          data: makePR({ number: 44 }),
+          fetchedAt: Date.now()
+        }
+      }
+    } as unknown as Partial<AppState>)
+
+    await expect(
+      store.getState().fetchPRForBranch(repoPath, branch, { force: true })
+    ).resolves.toMatchObject({ number: 44 })
+    expect(mockApi.gh.prForBranch).not.toHaveBeenCalled()
+    expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled()
+  })
 })

 describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
@@ -290,6 +320,7 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
          }
        ]
      },
+      worktreeCardProperties: ['pr'],
      prCache: {
        [`${repoPath}::${branch}`]: {
          data: makePR({ state: 'open' }),
@@ -311,6 +342,158 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
      priority: 80
    })
  })
+
+  it('does not enqueue active PR refresh when no PR-related surface is visible', () => {
+    const store = createTestStore()
+    const repoPath = '/repo'
+    const branch = 'feature/test'
+    const worktreeId = 'wt-1'
+
+    store.setState({
+      repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }],
+      groupBy: 'repo',
+      worktreeCardProperties: ['comment'],
+      rightSidebarOpen: false,
+      rightSidebarTab: 'source-control',
+      worktreesByRepo: {
+        'repo-1': [
+          {
+            id: worktreeId,
+            repoId: 'repo-1',
+            path: '/repo/worktrees/test',
+            branch,
+            displayName: 'test',
+            isMainWorktree: false,
+            isBare: false,
+            isArchived: false
+          }
+        ]
+      }
+    } as unknown as Partial<AppState>)
+
+    store.getState().refreshGitHubForWorktreeIfStale(worktreeId)
+
+    expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled()
+  })
+
+  it('skips active PR refresh IPC for SSH-backed repos', () => {
+    const store = createTestStore()
+    const repoPath = '/repo'
+    const branch = 'feature/test'
+    const worktreeId = 'wt-1'
+
+    store.setState({
+      repos: [
+        {
+          id: 'repo-1',
+          path: repoPath,
+          name: 'repo',
+          kind: 'git',
+          connectionId: 'ssh-1'
+        }
+      ],
+      groupBy: 'pr-status',
+      sshConnectionStates: new Map([['ssh-1', { status: 'connected' }]]),
+      worktreesByRepo: {
+        'repo-1': [
+          {
+            id: worktreeId,
+            repoId: 'repo-1',
+            path: '/repo/worktrees/test',
+            branch,
+            displayName: 'test',
+            isMainWorktree: false,
+            isBare: false,
+            isArchived: false
+          }
+        ]
+      }
+    } as unknown as Partial<AppState>)
+
+    store.getState().refreshGitHubForWorktreeIfStale(worktreeId)
+
+    expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled()
+  })
+
+  it('enqueues active PR refresh when source control is the visible PR surface', () => {
+    const store = createTestStore()
+    const repoPath = '/repo'
+    const branch = 'feature/test'
+    const worktreeId = 'wt-1'
+
+    store.setState({
+      repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }],
+      groupBy: 'repo',
+      worktreeCardProperties: ['comment'],
+      rightSidebarOpen: true,
+      rightSidebarTab: 'source-control',
+      worktreesByRepo: {
+        'repo-1': [
+          {
+            id: worktreeId,
+            repoId: 'repo-1',
+            path: '/repo/worktrees/test',
+            branch,
+            displayName: 'test',
+            isMainWorktree: false,
+            isBare: false,
+            isArchived: false
+          }
+        ]
+      }
+    } as unknown as Partial<AppState>)
+
+    store.getState().refreshGitHubForWorktreeIfStale(worktreeId)
+
+    expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({
+      candidate: expect.objectContaining({ repoPath, branch }),
+      reason: 'active',
+      priority: 80
+    })
+  })
+})
+
+describe('createGitHubSlice.refreshAllGitHub', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('refreshes stale PR data when source control is the visible PR surface', () => {
+    const store = createTestStore()
+    const repoPath = '/repo'
+    const branch = 'feature/test'
+
+    store.setState({
+      repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }],
+      groupBy: 'repo',
+      worktreeCardProperties: ['comment'],
+      rightSidebarOpen: true,
+      rightSidebarTab: 'source-control',
+      worktreesByRepo: {
+        'repo-1': [
+          {
+            id: 'wt-1',
+            repoId: 'repo-1',
+            path: '/repo/worktrees/test',
+            branch,
+            displayName: 'test',
+            isMainWorktree: false,
+            isBare: false,
+            isArchived: false,
+            lastActivityAt: 1
+          }
+        ]
+      }
+    } as unknown as Partial<AppState>)
+
+    store.getState().refreshAllGitHub()
+
+    expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({
+      candidate: expect.objectContaining({ repoPath, branch }),
+      reason: 'swr',
+      priority: 10
+    })
+  })
 })

 describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
@@ -394,6 +394,12 @@ function buildPRRefreshCandidate(
  const sshStatus = repo.connectionId
    ? state.sshConnectionStates.get(repo.connectionId)?.status
    : null
+  if (repo.connectionId) {
+    // Why: main currently runs gh against local repo paths only. Avoid sending
+    // remote worktrees into a queue that can only skip them until SSH gh support
+    // exists.
+    return null
+  }
  return {
    repoId: repo.id,
    repoPath: repoPath ?? repo.path,
@@ -1257,6 +1263,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
  fetchPRForBranch: async (repoPath, branch, options): Promise<PRInfo | null> => {
    const cacheKey = prCacheKey(repoPath, branch)
    const cached = get().prCache[cacheKey]
+    const repo = (get().repos ?? []).find((r) => r.path === repoPath)
+    if (repo?.connectionId) {
+      return cached?.data ?? null
+    }
    // Why: if a prior caller without a linkedPR cached `null` for this branch,
    // the worktree-card lookup (which has a linked PR fallback) would otherwise
    // return null forever. Refetch when the cached miss could now resolve via
@@ -1623,10 +1633,16 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
    const state = get()
    const now = Date.now()
    const stalePRCandidates: { candidate: GitHubPRRefreshCandidate; score: number }[] = []
+    const cardProps = state.worktreeCardProperties ?? []
+    const isPRStatusGrouping = state.groupBy === 'pr-status'
+    const rightSidebarShowsPR =
+      state.rightSidebarOpen &&
+      (state.rightSidebarTab === 'checks' || state.rightSidebarTab === 'source-control')
    const shouldRefreshPRs =
-      state.groupBy === 'pr-status' ||
-      state.worktreeCardProperties.includes('pr') ||
-      state.worktreeCardProperties.includes('ci')
+      isPRStatusGrouping ||
+      rightSidebarShowsPR ||
+      cardProps.includes('pr') ||
+      cardProps.includes('ci')

    for (const worktrees of Object.values(state.worktreesByRepo)) {
      for (const wt of worktrees) {
@@ -1660,7 +1676,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
        }
      }
    }
-    for (const { candidate } of stalePRCandidates.sort((a, b) => b.score - a.score).slice(0, 5)) {
+    const candidatesToRefresh = stalePRCandidates
+      .sort((a, b) => b.score - a.score)
+      .slice(0, isPRStatusGrouping ? stalePRCandidates.length : 5)
+    for (const { candidate } of candidatesToRefresh) {
      void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'swr', priority: 10 })
    }
  },
@@ -1833,8 +1852,15 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
    }

    const branch = worktree.branch.replace(/^refs\/heads\//, '')
+    const cardProps = state.worktreeCardProperties ?? []
+    const shouldRefreshPR =
+      state.groupBy === 'pr-status' ||
+      cardProps.includes('pr') ||
+      cardProps.includes('ci') ||
+      (state.rightSidebarOpen &&
+        (state.rightSidebarTab === 'checks' || state.rightSidebarTab === 'source-control'))

-    if (!worktree.isBare && branch) {
+    if (shouldRefreshPR && !worktree.isBare && branch) {
      const candidate = buildPRRefreshCandidate(state, worktree)
      if (candidate) {
        void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'active', priority: 80 })