From ebaa01e42c41c366db1872e5939f4337b6fcc699 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 6 Sep 2026 11:45:46 -0700 Subject: [PATCH] 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. --- .../use-branch-compare-refresh-triggers.ts | 88 +++++++++++ .../source-control/sync/use-branch-compare.ts | 144 +++++++----------- ...use-source-control-branch-compare.test.tsx | 143 ++++++++++++++++- 3 files changed, 282 insertions(+), 93 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare-refresh-triggers.ts diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare-refresh-triggers.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare-refresh-triggers.ts new file mode 100644 index 00000000000..781b70fe43d --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare-refresh-triggers.ts @@ -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> +}) { + const branchCompareStatusHeadRef = useRef(null) + const branchCompareRemoteStatusRef = useRef(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 + ]) +} diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts index 3979ddf8e35..e6159cfa9cd 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-branch-compare.ts @@ -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 - refreshBranchCompareRef: React.RefObject<() => Promise> -} { +}) { 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(null) const branchCompareRunPromiseRef = useRef | null>(null) + const branchCompareRecoveryPendingRef = useRef(false) const refreshBranchCompareRef = useRef<() => Promise>(async () => {}) + const recoverBranchCompareRef = useRef<() => Promise>(async () => {}) const startBranchCompareRef = useRef<(kind: BranchCompareRefreshKind) => Promise>( async () => {} ) @@ -58,8 +53,6 @@ export function useSourceControlBranchCompare({ const branchComparePollEnabledRef = useRef(false) const branchCompareLastRunEndedAtRef = useRef(-Infinity) const branchCompareLastRunDurationRef = useRef(0) - const branchCompareStatusHeadRef = useRef(null) - const branchCompareRemoteStatusRef = useRef(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 => { + 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 }) diff --git a/src/renderer/src/components/right-sidebar/use-source-control-branch-compare.test.tsx b/src/renderer/src/components/right-sidebar/use-source-control-branch-compare.test.tsx index ea32c80039b..23a426b66a5 100644 --- a/src/renderer/src/components/right-sidebar/use-source-control-branch-compare.test.tsx +++ b/src/renderer/src/components/right-sidebar/use-source-control-branch-compare.test.tsx @@ -9,7 +9,10 @@ const mocks = vi.hoisted(() => ({ beginGitBranchCompareRequest: vi.fn(), setGitBranchCompareResult: vi.fn(), clearGitBranchCompare: vi.fn(), - gitBranchCompareSummaryByWorktree: {} as Record + 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() 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() + mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise) + const root = await mount({ isBranchVisible: true, statusHead: 'head-1' }) + await act(async () => { + root.render() + 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( + + ) + }) + 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() + mocks.getRuntimeGitBranchCompare.mockReturnValueOnce(first.promise) + const root = await mount({ isBranchVisible: true, statusHead: 'head-1' }) + await act(async () => { + root.render() + await vi.advanceTimersByTimeAsync(90_000) + first.resolve(OK) + }) + await flush() + mocks.gitBranchCompareSummaryByWorktree = { + A: { baseRef: 'origin/main', status: 'ready' } + } + await act(async () => { + root.render() + }) + 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() + 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() + }) + await act(async () => { + root.render() + }) + } + 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() + 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() + }) + await act(async () => { + root.render() + }) + 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) +})