From 4f624e81ecdf06866144d0d7f384c90fabbc2fe1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:43:46 -0700 Subject: [PATCH] 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. --- .../right-sidebar/SourceControl.tsx | 3 +- .../source-control-dropdown-items.test.ts | 82 +++++++++++++++- ...-control-hosted-review-push-target.test.ts | 93 ++++++++++++++++++- ...ource-control-hosted-review-push-target.ts | 27 +++++- 4 files changed, 196 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 6a863e9a36d..a972c590c5b 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -1672,7 +1672,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..66615499f37 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,75 @@ describe('resolveDropdownItems', () => { expect(byKind.create_pr.hint).toBe('Run glab auth login in this environment') }) }) + +// Why: reproduce PR #8196 — a branch with a real git upstream and an open +// linked PR whose head is that branch had every remote row disabled because the +// unhydrated push target faked hasUpstream:false. Drive the same 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..bb0f52097c1 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,69 @@ 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: an open linked PR whose head is the checked-out branch must not stay + // blocked while its resolver-backed 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..08bbe5bc0a6 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 @@ -6,6 +6,7 @@ export function hasUsableHostedReviewPushTarget(args: { pushTarget?: GitPushTarget upstreamStatus?: GitUpstreamStatus hasResolvableHostedReviewPushTargetLink?: boolean + branchName?: string }): boolean { if (args.pushTarget) { return ( @@ -14,13 +15,33 @@ 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 resolver-backed link normally needs hydrated target metadata to + // prove which head it pushes to. But 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. Fork/cross-repo + // reviews track a differently-named head, so a mismatched (or missing) + // upstream stays blocked until the resolver proves the real target. + return ( + args.upstreamStatus?.hasUpstream === true && + upstreamTracksBranch(args.upstreamStatus.upstreamName, args.branchName) + ) } return args.upstreamStatus?.hasConfiguredPushTarget === true } +function upstreamTracksBranch( + upstreamName: string | undefined, + branchName: string | undefined +): boolean { + if (!upstreamName || !branchName) { + return false + } + // Why: upstreamName is `/`; remote names cannot contain `/`, + // so the branch is everything past the first slash (branches may contain `/`). + const separatorIndex = upstreamName.indexOf('/') + return separatorIndex >= 0 && upstreamName.slice(separatorIndex + 1) === branchName +} + function isResolvableHostedReviewNumber(value: unknown): value is number { return typeof value === 'number' && Number.isInteger(value) && value > 0 }