diff --git a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte index 9671c404..47019e1c 100644 --- a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte @@ -33,6 +33,7 @@ type CompletedPushOutcome, } from './branchCardHelpers'; import { buildPrButtonTitle } from './prButtonTooltip'; + import { shouldShowPushChanges } from './prButtonGitState'; import { getPreferredAgent } from '../settings/preferences.svelte'; import { agentState, REMOTE_AGENTS } from '../agents/agent.svelte'; import { prStateStore, type PrState } from '../../stores/prState.svelte'; @@ -106,17 +107,17 @@ let prStatusDraft = $state(null); let failedChecks = $state([]); - // Derive hasUnpushed by comparing the latest timeline commit SHA with the PR head SHA. - // This replaces the old approach of shelling out to `git rev-list`. - let hasUnpushed = $derived.by(() => { - if (!branch.prNumber || !timeline) return false; - // Don't show push changes on merged PRs - if (branch.prState === 'MERGED') return false; - // Find the first commit with a real (non-empty) SHA — pending commits have sha: "" - const latestCommit = timeline.commits.find((c) => c.sha && c.sha.length > 0); - if (!latestCommit || !prHeadSha) return false; - return latestCommit.sha !== prHeadSha; - }); + // Derive hasUnpushed from the git-state upstream relation when available, + // falling back to a HEAD/PR SHA comparison only when upstream is missing + // (e.g. fork PRs without an `origin/` ref). + let hasUnpushed = $derived( + shouldShowPushChanges({ + prNumber: branch.prNumber, + prState: branch.prState, + prHeadSha, + gitState: timeline?.gitState ?? null, + }) + ); // Sync local PR status state when branch prop changes let syncedBranchId = $state(null); @@ -536,13 +537,12 @@ console.warn('[Staged] Failed to clear PR status after push:', e); } pushStateStore.setPushDone(branch.id); - // Optimistically update prHeadSha to the latest timeline commit - // so hasUnpushed becomes false immediately, before the next PR - // status refresh picks up the new head SHA from GitHub. - const latestCommit = timeline?.commits.find((c) => c.sha && c.sha.length > 0); - if (latestCommit) { - prHeadSha = latestCommit.sha; - } + // Refresh local git state so `upstream.relation` settles back to + // `inSync` (driving hasUnpushed to false) without optimistically + // mutating prHeadSha here. + commands.refreshBranchGitState(branch.id).catch((e) => { + console.warn('[Staged] Failed to refresh git state after push:', e); + }); // Immediately refresh PR status so checks update right away prPollingService.refreshNow(branch.projectId); setTimeout(() => { diff --git a/apps/staged/src/lib/features/branches/prButtonGitState.test.ts b/apps/staged/src/lib/features/branches/prButtonGitState.test.ts new file mode 100644 index 00000000..aeb5a3d8 --- /dev/null +++ b/apps/staged/src/lib/features/branches/prButtonGitState.test.ts @@ -0,0 +1,181 @@ +import { describe, expect, it } from 'vitest'; +import { shouldShowPushChanges } from './prButtonGitState'; +import type { BranchGitState, UpstreamRelation } from '../../types'; + +function makeGitState( + relation: UpstreamRelation, + overrides: Partial = {} +): BranchGitState { + return { + headSha: 'localsha0000000000000000000000000000000a', + currentBranch: 'feature', + detachedHead: false, + expectedBranchMatches: true, + upstream: { + ref: 'origin/feature', + exists: relation !== 'missing', + sha: relation === 'missing' ? null : 'upstreamsha000000000000000000000000000b', + relation, + ahead: relation === 'localAhead' || relation === 'diverged' ? 1 : 0, + behind: relation === 'originAhead' || relation === 'diverged' ? 1 : 0, + mergeBaseSha: null, + }, + base: { ref: 'main', sha: null, commitsSinceFork: 0 }, + worktree: { + dirty: false, + modified: 0, + added: 0, + deleted: 0, + untracked: 0, + conflicted: 0, + }, + fetch: { status: 'fresh', fetchedAt: null, error: null }, + ...overrides, + }; +} + +describe('shouldShowPushChanges', () => { + it('returns true when local is ahead of upstream', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('localAhead'), + }) + ).toBe(true); + }); + + it('returns true when local has diverged from upstream', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('diverged'), + }) + ).toBe(true); + }); + + it('returns false when origin is ahead of local (nothing to push)', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('originAhead'), + }) + ).toBe(false); + }); + + it('returns false when local is in sync with upstream', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('inSync'), + }) + ).toBe(false); + }); + + it('falls back to head SHA comparison when upstream is missing and SHAs differ', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('missing', { headSha: 'localsha' }), + }) + ).toBe(true); + }); + + it('falls back to head SHA comparison when upstream is missing and SHAs match', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'samesha', + gitState: makeGitState('missing', { headSha: 'samesha' }), + }) + ).toBe(false); + }); + + it('returns false when upstream is missing and there is no PR head SHA yet', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: null, + gitState: makeGitState('missing'), + }) + ).toBe(false); + }); + + it('returns false for merged PRs even when local is ahead', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'MERGED', + prHeadSha: 'prsha', + gitState: makeGitState('localAhead'), + }) + ).toBe(false); + }); + + it('returns false when the branch has no PR yet', () => { + expect( + shouldShowPushChanges({ + prNumber: null, + prState: null, + prHeadSha: null, + gitState: makeGitState('localAhead'), + }) + ).toBe(false); + }); + + it('returns false when git state has not loaded yet', () => { + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: null, + }) + ).toBe(false); + }); + + it('returns false when a different branch is checked out (localAhead would otherwise apply)', () => { + // User has switched to `main`; `upstream.relation` reflects main vs + // origin/main, not the PR's branch, so the helper must bail out. + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('localAhead', { + expectedBranchMatches: false, + currentBranch: 'main', + }), + }) + ).toBe(false); + }); + + it('returns false when a different branch is checked out and upstream is missing', () => { + // Without the expected-branch guard, the SHA fallback would compare + // the wrong-branch HEAD against the PR head SHA and almost certainly + // surface a spurious "needs push". + expect( + shouldShowPushChanges({ + prNumber: 42, + prState: 'OPEN', + prHeadSha: 'prsha', + gitState: makeGitState('missing', { + expectedBranchMatches: false, + currentBranch: 'main', + headSha: 'mainsha0000000000000000000000000000000c', + }), + }) + ).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/branches/prButtonGitState.ts b/apps/staged/src/lib/features/branches/prButtonGitState.ts new file mode 100644 index 00000000..a06d70a2 --- /dev/null +++ b/apps/staged/src/lib/features/branches/prButtonGitState.ts @@ -0,0 +1,39 @@ +import type { BranchGitState } from '../../types'; + +export interface ShouldShowPushChangesInput { + prNumber: number | null; + prState: string | null; + prHeadSha: string | null; + gitState: BranchGitState | null | undefined; +} + +export function shouldShowPushChanges(input: ShouldShowPushChangesInput): boolean { + const { prNumber, prState, prHeadSha, gitState } = input; + if (!prNumber) return false; + if (prState === 'MERGED') return false; + if (!gitState) return false; + // When a different branch is checked out, neither `upstream.relation` + // (computed against HEAD's upstream) nor `headSha` reflect this PR's + // branch, so the comparison would be meaningless. + if (!gitState.expectedBranchMatches) return false; + + switch (gitState.upstream.relation) { + case 'localAhead': + case 'diverged': + return true; + case 'inSync': + case 'originAhead': + return false; + case 'missing': { + // Fork PRs may not have origin/. Fall back to comparing local + // HEAD with the PR head SHA reported by GitHub. + if (!gitState.headSha || !prHeadSha) return false; + return gitState.headSha !== prHeadSha; + } + default: { + const _exhaustive: never = gitState.upstream.relation; + void _exhaustive; + return false; + } + } +}