mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 08:03:22 +00:00
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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<string, DropdownItem> {
|
||||
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)
|
||||
})
|
||||
})
|
||||
|
||||
+89
-4
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
+24
-3
@@ -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>/<branch>`; 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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user