Recover branch compare on visibility change (#19021)

* Recover branch compare on visibility change Add recovery mode that reuses cached branch comparison data when the window regains focus instead of clearing results and forcing a refresh. This preserves the diff display during operations like rebasing that may cause the window to go to the background. * Retry failed branch comparison results Cached branch comparison results with error status are now excluded from the cache-hit check, ensuring they are retried rather than silently reused. This fixes missing diffs during rebasing. * Decouple branch compare recovery from refresh kinds Recovery is now a dedicated callback invoked independently on visibility changes, rather than a refresh kind. This allows pending recoveries to queue during in-flight requests, improving handling when the window regains focus during rebasing or other operations.
2026-09-25 00:02:35 +00:00 · 2026-09-06 11:45:46 -07:00
parent 0c33f58e8a
commit ebaa01e42c
3 changed files with 282 additions and 93 deletions
@@ -0,0 +1,88 @@
+import { useEffect, useRef, type MutableRefObject } from 'react'
+import type { GitUpstreamStatus } from '../../../../../../shared/git-status-types'
+import {
+  shouldRefreshBranchCompareForRemoteStatus,
+  shouldRefreshBranchCompareForStatusHead,
+  type BranchCompareRemoteStatusSnapshot,
+  type BranchCompareStatusHeadSnapshot
+} from './compare-summary'
+
+export function useBranchCompareRefreshTriggers({
+  activeWorktreeId,
+  worktreePath,
+  compareBaseRef,
+  isFolder,
+  isBranchVisible,
+  activeGitStatusHead,
+  remoteStatus,
+  refreshBranchCompareRef
+}: {
+  activeWorktreeId: string | null
+  worktreePath: string | null
+  compareBaseRef: string | null
+  isFolder: boolean
+  isBranchVisible: boolean
+  activeGitStatusHead: string | null
+  remoteStatus: GitUpstreamStatus | undefined
+  refreshBranchCompareRef: MutableRefObject<() => Promise<void>>
+}) {
+  const branchCompareStatusHeadRef = useRef<BranchCompareStatusHeadSnapshot | null>(null)
+  const branchCompareRemoteStatusRef = useRef<BranchCompareRemoteStatusSnapshot | null>(null)
+
+  useEffect(() => {
+    if (!activeWorktreeId || !worktreePath || !isBranchVisible || !compareBaseRef || isFolder) {
+      branchCompareStatusHeadRef.current = null
+      return
+    }
+    const current = {
+      baseRef: compareBaseRef,
+      statusHead: activeGitStatusHead,
+      worktreeId: activeWorktreeId
+    }
+    const previous = branchCompareStatusHeadRef.current
+    branchCompareStatusHeadRef.current = current
+    if (shouldRefreshBranchCompareForStatusHead(previous, current)) {
+      void refreshBranchCompareRef.current()
+    }
+  }, [
+    activeGitStatusHead,
+    activeWorktreeId,
+    compareBaseRef,
+    isBranchVisible,
+    isFolder,
+    refreshBranchCompareRef,
+    worktreePath
+  ])
+
+  useEffect(() => {
+    if (!activeWorktreeId || !worktreePath || !isBranchVisible || !compareBaseRef || isFolder) {
+      branchCompareRemoteStatusRef.current = null
+      return
+    }
+    // Why: pushing a branch can move its remote base and ahead count without changing local HEAD, which the HEAD-change effect alone misses.
+    const current = {
+      ahead: remoteStatus?.ahead ?? null,
+      baseRef: compareBaseRef,
+      behind: remoteStatus?.behind ?? null,
+      hasUpstream: remoteStatus?.hasUpstream ?? null,
+      upstreamName: remoteStatus?.upstreamName ?? null,
+      worktreeId: activeWorktreeId
+    }
+    const previous = branchCompareRemoteStatusRef.current
+    branchCompareRemoteStatusRef.current = current
+    if (shouldRefreshBranchCompareForRemoteStatus(previous, current)) {
+      void refreshBranchCompareRef.current()
+    }
+  }, [
+    activeWorktreeId,
+    compareBaseRef,
+    isBranchVisible,
+    isFolder,
+    refreshBranchCompareRef,
+    remoteStatus?.ahead,
+    remoteStatus?.behind,
+    remoteStatus?.hasUpstream,
+    remoteStatus?.upstreamName,
+    worktreePath
+  ])
+}
@@ -3,15 +3,11 @@ import { installWindowVisibilityInterval } from '@/lib/window-visibility-interva
 import { getConnectionId } from '@/lib/connection-context'
 import { getRuntimeGitBranchCompare, type RuntimeGitContext } from '@/runtime/runtime-git-client'
 import { useAppStore } from '@/store'
+import { createLoadingBranchCompareSummary } from '@/store/slices/editor/git/branch-compare-state'
 import type { GitUpstreamStatus } from '../../../../../../shared/git-status-types'
 import { shouldClearBranchCompareForMissingBase } from './base-ref-resolution'
-import {
-  shouldRefreshBranchCompareForRemoteStatus,
-  shouldRefreshBranchCompareForStatusHead,
-  type BranchCompareRemoteStatusSnapshot,
-  type BranchCompareStatusHeadSnapshot
-} from './compare-summary'
 import { slowTaskRequiredIdleMs } from '../../coalesced-poll-runner'
+import { useBranchCompareRefreshTriggers } from './use-branch-compare-refresh-triggers'

 // Why: 30s poll — slow runs idle for their own duration; explicit commit/remote/manual/base-ref refreshes still run immediately.
 export const BRANCH_REFRESH_INTERVAL_MS = 30_000
@@ -40,17 +36,16 @@ export function useSourceControlBranchCompare({
  isBranchVisible: boolean
  activeGitStatusHead: string | null
  remoteStatus: GitUpstreamStatus | undefined
-}): {
-  refreshBranchCompare: () => Promise<void>
-  refreshBranchCompareRef: React.RefObject<() => Promise<void>>
-} {
+}) {
  const beginGitBranchCompareRequest = useAppStore((s) => s.beginGitBranchCompareRequest)
  const setGitBranchCompareResult = useAppStore((s) => s.setGitBranchCompareResult)
  const clearGitBranchCompare = useAppStore((s) => s.clearGitBranchCompare)
  const branchCompareInFlightRef = useRef(false)
  const branchCompareRerunRef = useRef<BranchCompareRefreshKind | null>(null)
  const branchCompareRunPromiseRef = useRef<Promise<void> | null>(null)
+  const branchCompareRecoveryPendingRef = useRef(false)
  const refreshBranchCompareRef = useRef<() => Promise<void>>(async () => {})
+  const recoverBranchCompareRef = useRef<() => Promise<void>>(async () => {})
  const startBranchCompareRef = useRef<(kind: BranchCompareRefreshKind) => Promise<void>>(
    async () => {}
  )
@@ -58,8 +53,6 @@ export function useSourceControlBranchCompare({
  const branchComparePollEnabledRef = useRef(false)
  const branchCompareLastRunEndedAtRef = useRef(-Infinity)
  const branchCompareLastRunDurationRef = useRef(0)
-  const branchCompareStatusHeadRef = useRef<BranchCompareStatusHeadSnapshot | null>(null)
-  const branchCompareRemoteStatusRef = useRef<BranchCompareRemoteStatusSnapshot | null>(null)

  const runBranchCompare = useCallback(
    async (kind: BranchCompareRefreshKind) => {
@@ -67,27 +60,19 @@ export function useSourceControlBranchCompare({
        return
      }
      const requestKey = `${activeWorktreeId}:${compareBaseRef}:${Date.now()}`
-      const existingSummary =
-        useAppStore.getState().gitBranchCompareSummaryByWorktree[activeWorktreeId]
-      // Why: only reset to 'loading' on the first request or a base-ref change; resetting on every poll caused a visible loading→error→loading flicker.
-      const baseRefChanged = existingSummary && existingSummary.baseRef !== compareBaseRef
-      const shouldResetToLoading = !existingSummary || baseRefChanged
-      if (shouldResetToLoading) {
-        beginGitBranchCompareRequest(activeWorktreeId, requestKey, compareBaseRef)
-      } else {
-        beginGitBranchCompareRequest(activeWorktreeId, requestKey, compareBaseRef, {
-          preserveExistingSummary: true
-        })
-      }
+      const summary = useAppStore.getState().gitBranchCompareSummaryByWorktree[activeWorktreeId]
+      // Why: polling should preserve results unless the comparison base changed.
+      beginGitBranchCompareRequest(activeWorktreeId, requestKey, compareBaseRef, {
+        preserveExistingSummary: !!summary && summary.baseRef === compareBaseRef
+      })
      try {
-        const connectionId = getConnectionId(activeWorktreeId) ?? undefined
        const result = await getRuntimeGitBranchCompare(
          {
            // Why: route the branch compare by the repo OWNER host, not the focused runtime.
            settings: activeRepoSettings,
            worktreeId: activeWorktreeId,
            worktreePath,
-            connectionId
+            connectionId: getConnectionId(activeWorktreeId) ?? undefined
          },
          compareBaseRef,
          kind === 'interval' ? 'background' : 'interactive'
@@ -96,12 +81,8 @@ export function useSourceControlBranchCompare({
      } catch (error) {
        setGitBranchCompareResult(activeWorktreeId, requestKey, {
          summary: {
-            baseRef: compareBaseRef,
-            baseOid: null,
+            ...createLoadingBranchCompareSummary(compareBaseRef),
            compareRef: branchName,
-            headOid: null,
-            mergeBase: null,
-            changedFiles: 0,
            status: 'error',
            errorMessage: error instanceof Error ? error.message : 'Branch compare failed'
          },
@@ -154,11 +135,14 @@ export function useSourceControlBranchCompare({

  const startBranchCompare = useCallback(
    async (kind: BranchCompareRefreshKind) => {
-      if (kind === 'immediate') {
+      if (kind !== 'interval') {
        clearBranchComparePollTimer()
      }
      if (branchCompareInFlightRef.current) {
-        if (kind === 'immediate' || branchCompareRerunRef.current === null) {
+        if (
+          branchCompareRerunRef.current !== 'immediate' &&
+          (kind !== 'interval' || branchCompareRerunRef.current === null)
+        ) {
          branchCompareRerunRef.current = kind
        }
        return branchCompareRunPromiseRef.current ?? undefined
@@ -189,21 +173,23 @@ export function useSourceControlBranchCompare({
          branchCompareInFlightRef.current = false
          const rerunKind = branchCompareRerunRef.current
          branchCompareRerunRef.current = null
+          const recoveryPending = branchCompareRecoveryPendingRef.current
+          branchCompareRecoveryPendingRef.current = false
          if (rerunKind === 'immediate') {
            await refreshBranchCompareRef.current()
+          } else if (recoveryPending) {
+            await recoverBranchCompareRef.current()
          } else if (rerunKind === 'interval') {
            scheduleBranchComparePoll()
          }
        }
      })()
      branchCompareRunPromiseRef.current = runPromise
-      try {
-        await runPromise
-      } finally {
+      await runPromise.finally(() => {
        if (branchCompareRunPromiseRef.current === runPromise) {
          branchCompareRunPromiseRef.current = null
        }
-      }
+      })
    },
    [clearBranchComparePollTimer, runBranchCompare, scheduleBranchComparePoll]
  )
@@ -211,66 +197,41 @@ export function useSourceControlBranchCompare({
    () => startBranchCompare('immediate'),
    [startBranchCompare]
  )
+  const recoverBranchCompare = useCallback((): Promise<void> => {
+    const summary = useAppStore.getState().gitBranchCompareSummaryByWorktree[activeWorktreeId ?? '']
+    // Why: an in-flight result may recover visible data; loading, missing, changed-base, and failed results retry immediately.
+    if (
+      summary &&
+      summary.status !== 'loading' &&
+      summary.status !== 'error' &&
+      summary.baseRef === compareBaseRef
+    ) {
+      scheduleBranchComparePoll()
+      return Promise.resolve()
+    }
+    if (branchCompareInFlightRef.current) {
+      branchCompareRecoveryPendingRef.current = true
+      return branchCompareRunPromiseRef.current ?? Promise.resolve()
+    }
+    return refreshBranchCompareRef.current()
+  }, [activeWorktreeId, compareBaseRef, scheduleBranchComparePoll])
  // Why: publish in an effect, not the render body — a discarded render must not install its callback. Declared first so the effects below see the fresh one.
  useEffect(() => {
    refreshBranchCompareRef.current = refreshBranchCompare
+    recoverBranchCompareRef.current = recoverBranchCompare
    startBranchCompareRef.current = startBranchCompare
-  }, [refreshBranchCompare, startBranchCompare])
+  }, [recoverBranchCompare, refreshBranchCompare, startBranchCompare])

-  useEffect(() => {
-    if (!activeWorktreeId || !worktreePath || !isBranchVisible || !compareBaseRef || isFolder) {
-      branchCompareStatusHeadRef.current = null
-      return
-    }
-    const current = {
-      baseRef: compareBaseRef,
-      statusHead: activeGitStatusHead,
-      worktreeId: activeWorktreeId
-    }
-    const previous = branchCompareStatusHeadRef.current
-    branchCompareStatusHeadRef.current = current
-    if (shouldRefreshBranchCompareForStatusHead(previous, current)) {
-      void refreshBranchCompareRef.current()
-    }
-  }, [
+  useBranchCompareRefreshTriggers({
+    activeWorktreeId,
+    worktreePath,
+    compareBaseRef,
+    isFolder,
+    isBranchVisible,
    activeGitStatusHead,
-    activeWorktreeId,
-    compareBaseRef,
-    isBranchVisible,
-    isFolder,
-    worktreePath
-  ])
-
-  useEffect(() => {
-    if (!activeWorktreeId || !worktreePath || !isBranchVisible || !compareBaseRef || isFolder) {
-      branchCompareRemoteStatusRef.current = null
-      return
-    }
-    // Why: pushing a branch can move its remote base and ahead count without changing local HEAD, which the HEAD-change effect alone misses.
-    const current = {
-      ahead: remoteStatus?.ahead ?? null,
-      baseRef: compareBaseRef,
-      behind: remoteStatus?.behind ?? null,
-      hasUpstream: remoteStatus?.hasUpstream ?? null,
-      upstreamName: remoteStatus?.upstreamName ?? null,
-      worktreeId: activeWorktreeId
-    }
-    const previous = branchCompareRemoteStatusRef.current
-    branchCompareRemoteStatusRef.current = current
-    if (shouldRefreshBranchCompareForRemoteStatus(previous, current)) {
-      void refreshBranchCompareRef.current()
-    }
-  }, [
-    activeWorktreeId,
-    compareBaseRef,
-    isBranchVisible,
-    isFolder,
-    remoteStatus?.ahead,
-    remoteStatus?.behind,
-    remoteStatus?.hasUpstream,
-    remoteStatus?.upstreamName,
-    worktreePath
-  ])
+    remoteStatus,
+    refreshBranchCompareRef
+  })

  useEffect(() => {
    if (!activeWorktreeId || !worktreePath || !isBranchVisible || !compareBaseRef || isFolder) {
@@ -280,6 +241,7 @@ export function useSourceControlBranchCompare({
    branchComparePollEnabledRef.current = true
    const stopInterval = installWindowVisibilityInterval({
      run: () => void startBranchCompareRef.current('interval'),
+      runOnVisible: () => void recoverBranchCompareRef.current(),
      jitterOnVisible: true,
      intervalMs: BRANCH_REFRESH_INTERVAL_MS
    })
@@ -9,7 +9,10 @@ const mocks = vi.hoisted(() => ({
  beginGitBranchCompareRequest: vi.fn(),
  setGitBranchCompareResult: vi.fn(),
  clearGitBranchCompare: vi.fn(),
-  gitBranchCompareSummaryByWorktree: {} as Record<string, { baseRef: string } | undefined>
+  gitBranchCompareSummaryByWorktree: {} as Record<
+    string,
+    { baseRef: string; status?: string } | undefined
+  >
 }))

 vi.mock('@/runtime/runtime-git-client', () => ({
@@ -235,6 +238,9 @@ describe('useSourceControlBranchCompare scheduler', () => {
    vi.useFakeTimers()
    const first = deferred<typeof OK>()
    mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise)
+    mocks.gitBranchCompareSummaryByWorktree = {
+      A: { baseRef: 'origin/main', status: 'ready' }
+    }
    // Visible mounts run once immediately through the visibility interval.
    await mount({ isBranchVisible: true })
    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(1)
@@ -296,7 +302,8 @@ describe('useSourceControlBranchCompare scheduler', () => {
    expect(mocks.beginGitBranchCompareRequest).toHaveBeenLastCalledWith(
      'A',
      expect.any(String),
-      'origin/dev'
+      'origin/dev',
+      { preserveExistingSummary: false }
    )
  })

@@ -363,3 +370,135 @@ describe('useSourceControlBranchCompare scheduler', () => {
    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(2)
  })
 })
+
+describe('branch comparison visibility recovery', () => {
+  it.each(['loading', 'missing', 'base-change', 'error'])(
+    'bypasses slow polling backoff for %s data',
+    async (reason) => {
+      vi.useFakeTimers()
+      const first = deferred<typeof OK>()
+      mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise)
+      const root = await mount({ isBranchVisible: true, statusHead: 'head-1' })
+      await act(async () => {
+        root.render(<Probe isBranchVisible={false} statusHead="head-1" />)
+        await vi.advanceTimersByTimeAsync(90_000)
+        first.resolve(OK)
+      })
+      await flush()
+      mocks.gitBranchCompareSummaryByWorktree =
+        reason === 'missing'
+          ? {}
+          : {
+              A: {
+                baseRef: 'origin/main',
+                status: reason === 'loading' ? 'loading' : reason === 'error' ? 'error' : 'ready'
+              }
+            }
+      await act(async () => {
+        root.render(
+          <Probe
+            isBranchVisible
+            statusHead="head-2"
+            compareBaseRef={reason === 'base-change' ? 'origin/dev' : 'origin/main'}
+          />
+        )
+      })
+      await flush()
+      expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(2)
+      expect(mocks.getRuntimeGitBranchCompare).toHaveBeenLastCalledWith(
+        expect.objectContaining({ worktreeId: 'A' }),
+        reason === 'base-change' ? 'origin/dev' : 'origin/main',
+        'interactive'
+      )
+    }
+  )
+
+  it('preserves slow polling backoff when reopening valid data', async () => {
+    vi.useFakeTimers()
+    const first = deferred<typeof OK>()
+    mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise)
+    const root = await mount({ isBranchVisible: true, statusHead: 'head-1' })
+    await act(async () => {
+      root.render(<Probe isBranchVisible={false} statusHead="head-1" />)
+      await vi.advanceTimersByTimeAsync(90_000)
+      first.resolve(OK)
+    })
+    await flush()
+    mocks.gitBranchCompareSummaryByWorktree = {
+      A: { baseRef: 'origin/main', status: 'ready' }
+    }
+    await act(async () => {
+      root.render(<Probe isBranchVisible statusHead="head-1" />)
+    })
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(89_999)
+    })
+    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(1)
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(1)
+    })
+    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(2)
+    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenLastCalledWith(
+      expect.objectContaining({ worktreeId: 'A' }),
+      'origin/main',
+      'background'
+    )
+  })
+
+  it('coalesces rapid stale reopenings behind a slow request', async () => {
+    const first = deferred<typeof OK>()
+    mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise)
+    const root = await mount({ isBranchVisible: true, statusHead: 'head-1' })
+    mocks.gitBranchCompareSummaryByWorktree = {
+      A: { baseRef: 'origin/main', status: 'loading' }
+    }
+    for (let i = 0; i < 5; i++) {
+      await act(async () => {
+        root.render(<Probe isBranchVisible={false} statusHead="head-2" />)
+      })
+      await act(async () => {
+        root.render(<Probe isBranchVisible statusHead="head-2" />)
+      })
+    }
+    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(1)
+    await act(async () => {
+      first.resolve(OK)
+    })
+    await flush()
+    expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(2)
+  })
+})
+
+it('reuses a recovered in-flight result after reopening instead of immediately comparing twice', async () => {
+  vi.useFakeTimers()
+  const first = deferred<typeof OK>()
+  mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise)
+  const root = await mount({ isBranchVisible: true, statusHead: 'head-1' })
+  mocks.gitBranchCompareSummaryByWorktree = {
+    A: { baseRef: 'origin/main', status: 'loading' }
+  }
+  await act(async () => {
+    root.render(<Probe isBranchVisible={false} statusHead="head-1" />)
+  })
+  await act(async () => {
+    root.render(<Probe isBranchVisible statusHead="head-1" />)
+  })
+  mocks.setGitBranchCompareResult.mockImplementation(() => {
+    mocks.gitBranchCompareSummaryByWorktree = {
+      A: { baseRef: 'origin/main', status: 'ready' }
+    }
+  })
+  await act(async () => {
+    first.resolve(OK)
+  })
+  await flush()
+  expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(1)
+  await act(async () => {
+    await vi.advanceTimersByTimeAsync(BRANCH_REFRESH_INTERVAL_MS - 1)
+  })
+  expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(1)
+  await act(async () => {
+    await vi.advanceTimersByTimeAsync(1)
+  })
+  expect(mocks.getRuntimeGitBranchCompare).toHaveBeenCalledTimes(2)
+})