From 6a176b89fc3844848cb6519e1f0cc57d60d005af Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:45:32 -0700 Subject: [PATCH] fix(source-control): keep push enabled for same-repo linked review with real upstream (#8202) * fix(source-control): keep push enabled for same-repo linked review with real upstream Push/Force Push (and Pull/Sync/Publish) were wrongly disabled when a branch had a genuine git upstream but an open linked PR whose push target had not yet hydrated. `hasUsableHostedReviewPushTarget` hard-returned false for any resolvable review link without a hydrated `pushTarget`, so `resolveHostedReviewActionUpstreamStatus` synthesized `hasUpstream:false` and the whole remote menu treated the branch as unpublished. A same-repo review's head IS the checked-out branch, so a real upstream already tracking that branch is that head and is safe to use before the resolver hydrates a target. Treat that upstream as usable; keep blocking the fork-head/cross-repo case (upstream tracks a differently-named head) and the no-upstream case until the resolver proves the real target. Provider-agnostic (GitHub PR / GitLab MR) and covers the SSH linked-review path. Adds regression tests to source-control-hosted-review-push-target.test.ts and a composed pipeline test in source-control-dropdown-items.test.ts. * docs(source-control): trim verbose comments on linked-review push-target fix Co-authored-by: Orca * refactor(source-control): use canonical gitRefTargetsBranchName for linked-review upstream match Replace the hand-rolled upstreamTracksBranch leaf-split with the shared gitRefTargetsBranchName primitive, which trims and rejects malformed remote-qualified refs. The remote stays unknown until the resolver hydrates the push target, so this remains a branch-leaf match; the strict remote+branch check takes over once the target is known. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../right-sidebar/SourceControl.tsx | 3 +- .../source-control-dropdown-items.test.ts | 80 +++++++++++++++- ...-control-hosted-review-push-target.test.ts | 92 ++++++++++++++++++- ...ource-control-hosted-review-push-target.ts | 15 ++- 4 files changed, 181 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 22e3878848b..6ab53ec36ed 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -1676,7 +1676,8 @@ function SourceControlInner(): React.JSX.Element { const canUseHostedReviewPushTarget = hasUsableHostedReviewPushTarget({ pushTarget: activeWorktree?.pushTarget, upstreamStatus: remoteStatus, - hasResolvableHostedReviewPushTargetLink: hasResolvableReviewPushTargetLink + hasResolvableHostedReviewPushTargetLink: hasResolvableReviewPushTargetLink, + branchName }) const hostedReviewStateForActions = resolveHostedReviewStateForActions({ hostedReviewState: hostedReview?.state ?? null, 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 44aeb8e7bbc..86af5bafdb7 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 @@ -1,5 +1,13 @@ import { describe, expect, it } from 'vitest' -import { resolveDropdownItems, type DropdownActionInputs } from './source-control-dropdown-items' +import { + resolveDropdownItems, + type DropdownActionInputs, + type DropdownItem +} from './source-control-dropdown-items' +import { + hasUsableHostedReviewPushTarget, + resolveHostedReviewActionUpstreamStatus +} from './source-control-hosted-review-push-target' // Why: a shared defaults object keeps each case row terse while making the // "this is the one knob that differs from the baseline" intent obvious. @@ -768,3 +776,73 @@ describe('resolveDropdownItems', () => { expect(byKind.create_pr.hint).toBe('Run glab auth login in this environment') }) }) + +// Why: PR #8196 — drive the real push-target resolution the component uses so +// the whole chain stays regression-proof. +describe('resolveDropdownItems with an unhydrated linked-review push target', () => { + function pipeline(args: { + branchName: string + upstreamStatus: DropdownActionInputs['upstreamStatus'] + }): Record { + const canUseHostedReviewPushTarget = hasUsableHostedReviewPushTarget({ + // pushTarget intentionally omitted: the resolver has not hydrated it yet. + hasResolvableHostedReviewPushTargetLink: true, + branchName: args.branchName, + upstreamStatus: args.upstreamStatus + }) + const upstreamStatus = resolveHostedReviewActionUpstreamStatus({ + hasHostedReviewLink: true, + hasResolvableHostedReviewPushTargetLink: true, + hostedReviewState: 'open', + isHostedReviewStateLoading: false, + canUseHostedReviewPushTarget, + upstreamStatus: args.upstreamStatus + }) + const items = resolveDropdownItems( + inputs({ + stagedCount: 1, + hasMessage: true, + branchCommitsAhead: 7, + prState: 'open', + upstreamStatus, + canPushLinkedReviewWithoutUpstream: canUseHostedReviewPushTarget + }) + ) + return Object.fromEntries( + items.filter((e): e is DropdownItem => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + } + + it('enables Push and Force Push when the real upstream is the same-repo review head', () => { + const byKind = pipeline({ + branchName: 'mobile-resume-suspected-fixes', + upstreamStatus: { + hasUpstream: true, + upstreamName: 'origin/mobile-resume-suspected-fixes', + ahead: 7, + behind: 2 + } + }) + expect(byKind.push.disabled).toBe(false) + expect(byKind.push.title).not.toBe('Linked review branch target is unavailable') + expect(byKind.force_push.disabled).toBe(false) + expect(byKind.pull.disabled).toBe(false) + expect(byKind.sync.disabled).toBe(false) + expect(byKind.publish.disabled).toBe(true) + }) + + it('still blocks Push when the real upstream is an unrelated fork/helper head', () => { + const byKind = pipeline({ + branchName: 'mobile-resume-suspected-fixes', + upstreamStatus: { + hasUpstream: true, + upstreamName: 'origin/helper-branch', + ahead: 1, + behind: 0 + } + }) + expect(byKind.push.disabled).toBe(true) + expect(byKind.push.title).toBe('Linked review branch target is unavailable') + expect(byKind.force_push.disabled).toBe(true) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.test.ts b/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.test.ts index 8e732f68fff..b65a6f8e35d 100644 --- a/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.test.ts @@ -136,7 +136,12 @@ describe('hasPositiveHostedReviewNumberLink', () => { expect(hasPositiveHostedReviewNumberLink({ linkedBitbucketPR: 34 })).toBe(true) expect(hasPositiveHostedReviewNumberLink({ linkedAzureDevOpsPR: 56 })).toBe(true) expect(hasPositiveHostedReviewNumberLink({ linkedGiteaPR: 78 })).toBe(true) - expect(hasPositiveHostedReviewNumberLink({ linkedGitHubPR: 0, linkedGitLabMR: -1 })).toBe(false) + expect( + hasPositiveHostedReviewNumberLink({ + linkedGitHubPR: 0, + linkedGitLabMR: -1 + }) + ).toBe(false) expect(hasPositiveHostedReviewNumberLink({ linkedGitHubPR: Number.NaN })).toBe(false) expect(hasPositiveHostedReviewNumberLink({})).toBe(false) }) @@ -187,18 +192,33 @@ describe('hasUsableHostedReviewPushTarget', () => { expect( hasUsableHostedReviewPushTarget({ pushTarget: { remoteName: 'fork', branchName: 'feature' }, - upstreamStatus: { hasUpstream: true, upstreamName: 'fork/feature', ahead: 1, behind: 0 } + upstreamStatus: { + hasUpstream: true, + upstreamName: 'fork/feature', + ahead: 1, + behind: 0 + } }) ).toBe(true) expect( hasUsableHostedReviewPushTarget({ - upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0, hasConfiguredPushTarget: true } + upstreamStatus: { + hasUpstream: false, + ahead: 0, + behind: 0, + hasConfiguredPushTarget: true + } }) ).toBe(true) expect( hasUsableHostedReviewPushTarget({ hasResolvableHostedReviewPushTargetLink: true, - upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0, hasConfiguredPushTarget: true } + upstreamStatus: { + hasUpstream: false, + ahead: 0, + behind: 0, + hasConfiguredPushTarget: true + } }) ).toBe(false) expect( @@ -209,4 +229,68 @@ describe('hasUsableHostedReviewPushTarget', () => { ).toBe(false) expect(hasUsableHostedReviewPushTarget({ upstreamStatus: unrelatedUpstream })).toBe(false) }) + + it('treats a same-repo review upstream that already tracks the branch as usable', () => { + // Why: a same-repo review must not stay blocked while its push target is + // unhydrated — the real upstream already targets the review head. + expect( + hasUsableHostedReviewPushTarget({ + hasResolvableHostedReviewPushTargetLink: true, + branchName: 'feature/foo', + upstreamStatus: { + hasUpstream: true, + upstreamName: 'origin/feature/foo', + ahead: 7, + behind: 2 + } + }) + ).toBe(true) + }) + + it('keeps blocking a review whose upstream tracks an unrelated fork/helper head', () => { + expect( + hasUsableHostedReviewPushTarget({ + hasResolvableHostedReviewPushTargetLink: true, + branchName: 'feature', + upstreamStatus: unrelatedUpstream + }) + ).toBe(false) + }) + + it('keeps blocking a resolvable review with no real upstream', () => { + expect( + hasUsableHostedReviewPushTarget({ + hasResolvableHostedReviewPushTargetLink: true, + branchName: 'feature', + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } + }) + ).toBe(false) + }) +}) + +describe('resolveHostedReviewActionUpstreamStatus with a same-repo upstream', () => { + it('does not synthesize hasUpstream:false when the real upstream is the review head', () => { + const realUpstream = { + hasUpstream: true, + upstreamName: 'origin/mobile-resume-suspected-fixes', + ahead: 7, + behind: 2 + } + const canUseHostedReviewPushTarget = hasUsableHostedReviewPushTarget({ + hasResolvableHostedReviewPushTargetLink: true, + branchName: 'mobile-resume-suspected-fixes', + upstreamStatus: realUpstream + }) + expect(canUseHostedReviewPushTarget).toBe(true) + expect( + resolveHostedReviewActionUpstreamStatus({ + hasHostedReviewLink: true, + hasResolvableHostedReviewPushTargetLink: true, + hostedReviewState: 'open', + isHostedReviewStateLoading: false, + canUseHostedReviewPushTarget, + upstreamStatus: realUpstream + }) + ).toBe(realUpstream) + }) }) diff --git a/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.ts b/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.ts index 9dd07b7e27b..99be32e886e 100644 --- a/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.ts +++ b/src/renderer/src/components/right-sidebar/source-control-hosted-review-push-target.ts @@ -1,11 +1,13 @@ import type { GitPushTarget, GitUpstreamStatus } from '../../../../shared/types' import type { HostedReviewState } from '../../../../shared/hosted-review' import { getPublishTargetDisplayName } from '../../../../shared/git-publish-target-status' +import { gitRefTargetsBranchName } from '../../../../shared/git-remote-branch-name' export function hasUsableHostedReviewPushTarget(args: { pushTarget?: GitPushTarget upstreamStatus?: GitUpstreamStatus hasResolvableHostedReviewPushTargetLink?: boolean + branchName?: string }): boolean { if (args.pushTarget) { return ( @@ -14,9 +16,16 @@ export function hasUsableHostedReviewPushTarget(args: { ) } if (args.hasResolvableHostedReviewPushTargetLink) { - // Why: a bare branch-config flag does not identify which review head it will - // push to; resolver-backed links need hydrated target metadata to prove it. - return false + // Why: a same-repo review's head is the checked-out branch, so a real + // upstream tracking it is safe before the resolver hydrates. Fork/cross-repo + // heads differ, so a mismatched or missing upstream stays blocked. The + // review's remote is unknown pre-hydration, so this can only match the + // branch leaf; the strict remote+branch check above takes over once known. + return ( + args.upstreamStatus?.hasUpstream === true && + args.branchName !== undefined && + gitRefTargetsBranchName(args.upstreamStatus.upstreamName, args.branchName) + ) } return args.upstreamStatus?.hasConfiguredPushTarget === true }