From c762b06d13429d889741d851745745f5e65986da Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 19:01:50 -0700 Subject: [PATCH] Keep explicit push and force push actions available without upstream - Keep the explicit "Push" and "Force Push" dropdown options enabled when there is no upstream or the upstream status is loading, letting Git resolve the target at execution time. - Stop auto-upgrading regular push operations to force push in the background, ensuring normal push actions stay non-force. - Explicitly route the primary action and commit-and-push flows to the "force_push" action when a lease force push is required. --- .../right-sidebar/SourceControl.tsx | 47 +++++-- .../source-control-dropdown-items.test.ts | 82 +++++++++--- .../source-control-dropdown-items.ts | 118 +++++++++++------- 3 files changed, 171 insertions(+), 76 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index b097fab1358..0543f8a0a43 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -2429,7 +2429,6 @@ function SourceControlInner(): React.JSX.Element { | 'rebase', options?: { target?: SourceControlOperationTarget - remoteStatus?: GitUpstreamStatus baseRef?: string | null } ): Promise => { @@ -2461,18 +2460,17 @@ function SourceControlInner(): React.JSX.Element { return true } if (kind === 'push') { - const forceWithLease = shouldForcePushWithLeaseForUpstream( - options?.remoteStatus ?? remoteStatus - ) + // Why: kind 'push' must stay a regular push. Force-with-lease is only + // for kind 'force_push' (and callers that choose it). Auto-upgrading + // here made the always-enabled dropdown Push row silently force-push + // while its tooltip claimed a normal push might merely fail. await pushBranch( target.worktreeId, target.worktreePath, false, target.connectionId, target.pushTarget, - forceWithLease - ? { forceWithLease: true, runtimeTargetSettings: target.settings } - : { runtimeTargetSettings: target.settings } + { runtimeTargetSettings: target.settings } ) return true } @@ -2581,7 +2579,6 @@ function SourceControlInner(): React.JSX.Element { pushBranch, rebaseFromBase, refreshActiveGitStatusAfterMutation, - remoteStatus, syncBranch, worktreePath ] @@ -2695,9 +2692,20 @@ function SourceControlInner(): React.JSX.Element { if (!ok) { return } + // Why: "Commit & Force Push" still invokes remoteKind 'push' from the + // dropdown kind map. Route to force_push when the upstream shape requires + // lease force so compound commit does not silently fall back to a regular + // push after we stopped auto-upgrading kind 'push'. + if ( + remoteKind === 'push' && + shouldForcePushWithLeaseForUpstream(remoteStatusForActions ?? remoteStatus) + ) { + await runRemoteAction('force_push') + return + } await runRemoteAction(remoteKind) }, - [handleCommit, runRemoteAction] + [handleCommit, remoteStatus, remoteStatusForActions, runRemoteAction] ) const handlePullRequestCreated = useCallback( @@ -3951,7 +3959,6 @@ function SourceControlInner(): React.JSX.Element { }) const remoteOk = await runRemoteAction(remoteStep, { target: operationTarget, - remoteStatus: latestUpstreamStatus, baseRef: token.baseRef }) if (abortIfStale()) { @@ -4642,8 +4649,17 @@ function SourceControlInner(): React.JSX.Element { case 'stage': void handleStageAllPrimary() return - case 'commit': case 'push': + // Why: primary labels "Force Push" while keeping kind 'push'. Invoke the + // explicit force path when lease force is required so regular push stays + // non-force for the dropdown. + handleActionInvoke( + shouldForcePushWithLeaseForUpstream(remoteStatusForActions ?? remoteStatus) + ? 'force_push' + : 'push' + ) + return + case 'commit': case 'pull': case 'sync': case 'publish': @@ -4653,7 +4669,14 @@ function SourceControlInner(): React.JSX.Element { case 'create_pr_intent': void runCreatePrIntent() } - }, [handleActionInvoke, handleStageAllPrimary, primaryAction.kind, runCreatePrIntent]) + }, [ + handleActionInvoke, + handleStageAllPrimary, + primaryAction.kind, + remoteStatus, + remoteStatusForActions, + runCreatePrIntent + ]) const handleCreatePrHeaderClick = useCallback((): void => { if (!createPrHeaderAction || createPrHeaderAction.disabled) { diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts index 58f5c064351..44aeb8e7bbc 100644 --- a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts @@ -77,7 +77,7 @@ describe('resolveDropdownItems', () => { expect(byKind.commit.title).toBe('Commit staged changes') }) - it('disables push actions but keeps Fetch enabled when branch has no upstream', () => { + it('enables explicit push actions and keeps Fetch enabled when branch has no upstream', () => { const items = resolveDropdownItems( inputs({ stagedCount: 1, @@ -88,7 +88,8 @@ describe('resolveDropdownItems', () => { const byKind = Object.fromEntries( items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) ) - expect(byKind.push.disabled).toBe(true) + expect(byKind.push.disabled).toBe(false) + expect(byKind.force_push.disabled).toBe(false) expect(byKind.commit_push.disabled).toBe(true) expect(byKind.publish.disabled).toBe(false) expect(byKind.fetch.disabled).toBe(false) @@ -183,8 +184,8 @@ describe('resolveDropdownItems', () => { ) expect(byKind.push.label).toBe('Push (14)') - expect(byKind.push.disabled).toBe(true) - expect(byKind.push.title).toBe('Use Force Push — remote only has older copies of local commits') + expect(byKind.push.disabled).toBe(false) + expect(byKind.push.title).toBe('Try a regular push; git may require force push') expect(byKind.force_push.label).toBe('Force Push (4)') expect(byKind.force_push.disabled).toBe(false) expect(byKind.force_push.title).toBe( @@ -364,10 +365,11 @@ describe('resolveDropdownItems', () => { } }) - it('disables remote rows with a loading tooltip when upstreamStatus is undefined', () => { + it('keeps explicit push rows available while upstreamStatus is undefined', () => { // Why: mirrors the primary-action guard — while fetchUpstreamStatus is in // flight we must not let the user click Publish on an already-tracked - // branch (which would re-run `git push -u` and clobber the upstream). + // branch, but explicit Push/Force Push resolve their git target at click + // time and should remain available. const items = resolveDropdownItems( inputs({ stagedCount: 1, hasMessage: true, upstreamStatus: undefined }) ) @@ -377,8 +379,6 @@ describe('resolveDropdownItems', () => { const loadingBlocked = [ 'commit_push', 'commit_sync', - 'push', - 'force_push', 'pull', 'fast_forward', 'sync', @@ -391,21 +391,36 @@ describe('resolveDropdownItems', () => { // Commit itself does not depend on upstream — it remains enabled when // staged + message are present and no commit is in flight. expect(byKind.commit.disabled).toBe(false) + expect(byKind.push.disabled).toBe(false) + expect(byKind.push.title).toBe('Push this branch and set an upstream if needed') + expect(byKind.force_push.disabled).toBe(false) + expect(byKind.force_push.title).toBe( + 'Force push this branch with lease and set an upstream if needed.' + ) expect(byKind.fetch.disabled).toBe(false) expect(byKind.fetch.title).toBe('Fetch from remote without merging') }) - it('keeps Fetch enabled and surfaces publish-first tooltips when upstream is absent', () => { + it('keeps explicit push rows enabled and surfaces publish-first tooltips elsewhere', () => { // Why: sibling to the upstreamStatus=undefined test above. Once the fetch - // resolves to hasUpstream=false, the dropdown should explain that the - // user needs to publish first (rather than leaving the loading copy). + // resolves to hasUpstream=false, only pull/sync rows need publish-first + // copy; explicit push rows can set the upstream themselves. const items = resolveDropdownItems( - inputs({ upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } }) + inputs({ + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, + branchCommitsAhead: 2 + }) ) const byKind = Object.fromEntries( items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) ) - expect(byKind.push.title).toBe('Publish the branch first to push commits') + expect(byKind.push.title).toBe('Push this branch and set an upstream if needed') + expect(byKind.push.disabled).toBe(false) + expect(byKind.force_push.label).toBe('Force Push (2)') + expect(byKind.force_push.title).toBe( + 'Force push 2 branch commits with lease and set an upstream if needed.' + ) + expect(byKind.force_push.disabled).toBe(false) expect(byKind.pull.title).toBe('Publish the branch first to pull commits') expect(byKind.fast_forward.title).toBe('Publish the branch first to fast-forward') expect(byKind.sync.title).toBe('Publish the branch first to sync commits') @@ -482,7 +497,7 @@ describe('resolveDropdownItems', () => { expect(byKind.publish.disabled).toBe(false) }) - it('does not mention Publish Branch when the linked PR is already merged', () => { + it('keeps explicit push rows available when the linked PR is already merged', () => { const items = resolveDropdownItems( inputs({ upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, @@ -492,7 +507,12 @@ describe('resolveDropdownItems', () => { const byKind = Object.fromEntries( items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) ) - expect(byKind.push.title).toBe('PR is already merged') + expect(byKind.push.title).toBe('Push this branch and set an upstream if needed') + expect(byKind.push.disabled).toBe(false) + expect(byKind.force_push.title).toBe( + 'Force push this branch with lease and set an upstream if needed.' + ) + expect(byKind.force_push.disabled).toBe(false) expect(byKind.pull.title).toBe('PR is already merged') expect(byKind.fast_forward.title).toBe('PR is already merged') expect(byKind.sync.title).toBe('PR is already merged') @@ -518,12 +538,40 @@ describe('resolveDropdownItems', () => { expect(byKind.commit_push.disabled).toBe(false) expect(byKind.push.title).toBe('Push updates to the linked review branch') expect(byKind.push.disabled).toBe(false) + expect(byKind.force_push.label).toBe('Force Push (1)') + expect(byKind.force_push.disabled).toBe(false) expect(byKind.publish.label).toBe('Linked Review') expect(byKind.publish.title).toBe('Linked review branch already exists') expect(byKind.publish.disabled).toBe(true) }) - it('blocks Push when an open linked review has no upstream and no branch target', () => { + it('keeps explicit Push available for a linked review with a usable target and no commits ahead', () => { + // Why: branchCommitsAhead === 0 must not be confused with a missing review + // head. Primary Push stays enabled; dropdown Push/Force Push match that. + const items = resolveDropdownItems( + inputs({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, + branchCommitsAhead: 0, + prState: 'open', + canPushLinkedReviewWithoutUpstream: true + }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.push.disabled).toBe(false) + expect(byKind.push.title).toBe('Push this branch and set an upstream if needed') + expect(byKind.force_push.disabled).toBe(false) + expect(byKind.commit_push.disabled).toBe(false) + expect(byKind.publish.disabled).toBe(true) + }) + + it('blocks Push when an open linked review has no branch target', () => { + // Why: faked hasUpstream=false for an unusable review head must not re-open + // push against an unrelated configured upstream. Primary already blocks + // this; the always-allow Push rows keep the same target-safety gate. const items = resolveDropdownItems( inputs({ stagedCount: 1, @@ -540,6 +588,8 @@ describe('resolveDropdownItems', () => { expect(byKind.commit_push.title).toBe('Linked review branch target is unavailable') expect(byKind.push.title).toBe('Linked review branch target is unavailable') expect(byKind.push.disabled).toBe(true) + expect(byKind.force_push.title).toBe('Linked review branch target is unavailable') + expect(byKind.force_push.disabled).toBe(true) expect(byKind.publish.label).toBe('Linked Review') expect(byKind.publish.title).toBe('Linked review branch target is unavailable') expect(byKind.publish.disabled).toBe(true) diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts index dc31dcb6c0d..58bbfe9454e 100644 --- a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts @@ -95,6 +95,14 @@ function formatManualForcePushTitle(ahead: number, behind: number, upstreamName? return `Force push ${commitText} with lease to update ${upstreamName ?? 'the remote branch'}.` } +function formatUnpublishedForcePushTitle(branchCommitsAhead: number | undefined): string { + const countText = + branchCommitsAhead && branchCommitsAhead > 0 + ? `${branchCommitsAhead} branch commit${branchCommitsAhead === 1 ? '' : 's'}` + : 'this branch' + return `Force push ${countText} with lease and set an upstream if needed.` +} + function formatRebaseBaseRef(baseRef: string): string { return baseRef.replace(/^refs\/remotes\//, '').replace(/^remotes\//, '') } @@ -143,7 +151,11 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr // branch during the post-worktree-switch transient window, and a click there // would re-run `git push -u` and clobber the real upstream. Every // upstream-dependent row disables itself while loading so the primary - // button's stable-frame guarantee extends to the dropdown. + // button's stable-frame guarantee extends to the dropdown. Explicit + // Push/Force Push are the exception for loading/unpublished branches: the + // git command resolves its target itself. Linked-review rows without a + // usable push target still block — otherwise we would push an unrelated + // configured upstream while the menu claimed first-publish semantics. const upstreamLoading = upstreamStatus === undefined const hasUpstream = upstreamStatus?.hasUpstream ?? false const hasOpenHostedReview = prState === 'open' || prState === 'draft' @@ -153,8 +165,11 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr hasCurrentBranch && branchCommitsAhead !== 0 && canPushLinkedReviewWithoutUpstream + // Why: only the missing review head is a hard block. branchCommitsAhead === 0 + // still means the target is known — primary Push stays available, and + // explicit Push must match that rather than claiming "target unavailable". const pushBlockedByOpenHostedReviewTarget = - !hasUpstream && hasOpenHostedReview && !canPushUntrackedHostedReview + !hasUpstream && hasOpenHostedReview && !canPushLinkedReviewWithoutUpstream const publishBlockedByMergedPR = !hasUpstream && prState === 'merged' const publishBlockedByPRLoading = !hasUpstream && !!isPRStateLoading const publishBlockedByOpenHostedReview = !hasUpstream && hasOpenHostedReview @@ -162,8 +177,16 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr const ahead = upstreamStatus?.ahead ?? 0 const behind = upstreamStatus?.behind ?? 0 const shouldForcePushWithLease = shouldForcePushWithLeaseForUpstream(upstreamStatus) + // Why: force-push counts prefer branch-compare when upstream ahead is missing + // or misleading — unpublished/loading branches report ahead=0, and + // patch-equivalent rewrites inflate ahead far above real branch commits. + // On a normal tracked branch, upstream ahead is the correct force-push size. const pushLabelCount = - shouldForcePushWithLease && branchCommitsAhead !== undefined ? branchCommitsAhead : ahead + branchCommitsAhead !== undefined && + branchCommitsAhead > 0 && + (shouldForcePushWithLease || !hasUpstream) + ? branchCommitsAhead + : ahead const forcePushTitle = formatForcePushTitle(branchCommitsAhead, upstreamStatus?.upstreamName) const createReviewCopy = reviewCopy(hostedReviewCreation?.provider) @@ -216,7 +239,7 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr ? 'Check out a branch before pushing commits' : pushBlockedByOpenHostedReviewTarget ? 'Linked review branch target is unavailable' - : !hasUpstream && !canPushUntrackedHostedReview + : !hasUpstream && !(hasOpenHostedReview && canPushLinkedReviewWithoutUpstream) ? 'Publish the branch first to push commits' : (commitDisabledReason ?? (shouldForcePushWithLease @@ -228,10 +251,13 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr kind: 'commit_push', label: shouldForcePushWithLease ? 'Commit & Force Push' : 'Commit & Push', title: commitPushTitle, + // Why: match explicit Push — only an open linked review with a known head + // can commit+push without a git upstream. Missing heads still block; plain + // unpublished branches still need Publish first. disabled: globalBusy || upstreamLoading || - (!hasUpstream && !canPushUntrackedHostedReview) || + (!hasUpstream && !(hasOpenHostedReview && canPushLinkedReviewWithoutUpstream)) || publishBlockedByDetachedHead || publishBlockedByPRLoading || publishBlockedByMergedPR || @@ -284,54 +310,50 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr const pushItem: DropdownItem = { kind: 'push', label: formatCountLabel('Push', ahead), - title: upstreamLoading - ? 'Checking branch status…' - : publishBlockedByPRLoading - ? 'Checking PR status…' - : publishBlockedByMergedPR - ? 'PR is already merged' - : publishBlockedByDetachedHead - ? 'Check out a branch before pushing commits' - : canPushUntrackedHostedReview - ? 'Push updates to the linked review branch' - : pushBlockedByOpenHostedReviewTarget - ? 'Linked review branch target is unavailable' - : !hasUpstream - ? 'Publish the branch first to push commits' - : shouldForcePushWithLease - ? 'Use Force Push — remote only has older copies of local commits' - : behind > 0 && ahead > 0 - ? 'Push local commits; git may require syncing first' - : ahead === 0 - ? `Nothing to push${upstreamStatus?.upstreamName ? ` to ${upstreamStatus.upstreamName}` : ''}` - : describePushCount(ahead), - disabled: - globalBusy || - upstreamLoading || - (!hasUpstream && !canPushUntrackedHostedReview) || - publishBlockedByDetachedHead || - shouldForcePushWithLease + title: publishBlockedByDetachedHead + ? 'Check out a branch before pushing commits' + : pushBlockedByOpenHostedReviewTarget + ? 'Linked review branch target is unavailable' + : upstreamLoading + ? 'Push this branch and set an upstream if needed' + : canPushUntrackedHostedReview + ? 'Push updates to the linked review branch' + : !hasUpstream + ? 'Push this branch and set an upstream if needed' + : shouldForcePushWithLease + ? 'Try a regular push; git may require force push' + : behind > 0 && ahead > 0 + ? 'Push local commits; git may require syncing first' + : ahead === 0 + ? `Nothing to push${upstreamStatus?.upstreamName ? ` to ${upstreamStatus.upstreamName}` : ''}` + : describePushCount(ahead), + // Why: keep Push available without an upstream (git resolves --set-upstream + // itself) and even when force-with-lease is recommended — regular Push must + // stay a non-force path. Only block detached HEAD and linked reviews whose + // push target is still unknown (otherwise we would push an unrelated upstream). + disabled: globalBusy || publishBlockedByDetachedHead || pushBlockedByOpenHostedReviewTarget } const forcePushItem: DropdownItem = { kind: 'force_push', label: formatCountLabel('Force Push', pushLabelCount), - title: upstreamLoading - ? 'Checking branch status…' - : publishBlockedByPRLoading - ? 'Checking PR status…' - : publishBlockedByMergedPR - ? 'PR is already merged' - : publishBlockedByDetachedHead - ? 'Check out a branch before force pushing commits' - : !hasUpstream - ? 'Publish the branch first to force push commits' - : ahead === 0 - ? `Nothing to force push${upstreamStatus?.upstreamName ? ` to ${upstreamStatus.upstreamName}` : ''}` - : shouldForcePushWithLease - ? forcePushTitle - : formatManualForcePushTitle(ahead, behind, upstreamStatus?.upstreamName), - disabled: globalBusy || upstreamLoading || !hasUpstream || publishBlockedByDetachedHead + title: publishBlockedByDetachedHead + ? 'Check out a branch before force pushing commits' + : pushBlockedByOpenHostedReviewTarget + ? 'Linked review branch target is unavailable' + : upstreamLoading + ? formatUnpublishedForcePushTitle(branchCommitsAhead) + : !hasUpstream + ? formatUnpublishedForcePushTitle(branchCommitsAhead) + : pushLabelCount === 0 + ? `Nothing to force push${upstreamStatus?.upstreamName ? ` to ${upstreamStatus.upstreamName}` : ''}` + : shouldForcePushWithLease + ? forcePushTitle + : formatManualForcePushTitle(pushLabelCount, behind, upstreamStatus?.upstreamName), + // Why: same target-safety gate as Push — force-with-lease to a wrong or + // unresolved review head is worse than blocking the row until the target + // is known. Explicit Force Push stays available without an upstream. + disabled: globalBusy || publishBlockedByDetachedHead || pushBlockedByOpenHostedReviewTarget } const pullItem: DropdownItem = {