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 }