Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions apps/staged/src/lib/features/branches/BranchCardPrButton.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -106,17 +107,17 @@
let prStatusDraft = $state<boolean | null>(null);
let failedChecks = $state<PrFailedCheck[]>([]);

// 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/<branch>` 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<string | null>(null);
Expand Down Expand Up @@ -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);
});
Comment on lines +543 to +545
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Force-refresh git state after push completion

After a successful push, this now relies on refreshBranchGitState to flip hasUnpushed back to false, but that command is TTL-gated (FetchMode::Ttl, 30s) and can skip fetching if a refresh ran recently. Because git push does not update refs/remotes/origin/<branch> locally, skipping fetch leaves upstream.relation as localAhead, so the UI can keep showing "Push changes" even though the push succeeded. This is reproducible when the user pushes shortly after any prior git-state refresh.

Useful? React with 👍 / 👎.

// Immediately refresh PR status so checks update right away
prPollingService.refreshNow(branch.projectId);
setTimeout(() => {
Expand Down
181 changes: 181 additions & 0 deletions apps/staged/src/lib/features/branches/prButtonGitState.test.ts
Original file line number Diff line number Diff line change
@@ -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> = {}
): 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);
});
});
39 changes: 39 additions & 0 deletions apps/staged/src/lib/features/branches/prButtonGitState.ts
Original file line number Diff line number Diff line change
@@ -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/<branch>. 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;
}
}
}