Confirm before merging a PR/MR from the sidebar strategy dropdown (#8911)

The right-sidebar Checks panel merge control merged the PR/MR immediately when
a strategy was chosen from its dropdown, with no confirmation — so opening the
dropdown to switch strategies (e.g. Squash -> Merge commit) merged on the spot.
Every other PR/MR merge surface (PullRequestPage, GitHubItemDialog, TaskPage)
already confirms first, and the close/reopen path in this same hook confirms too;
only the hosted-review merge path was missing the gate.

Add the same confirmation dialog to the hosted-review handleMerge, provider-aware
so it reads "Squash and merge PR #N?" / "Squash and merge MR !N?". Selecting a
strategy now asks before merging; the merge still runs when explicitly confirmed.

Fixes #7943
This commit is contained in:
Brennan Benson
2026-07-19 13:09:36 -07:00
committed by GitHub
parent 500f0c50de
commit 37cd830192
7 changed files with 142 additions and 8 deletions
@@ -46,14 +46,25 @@ function makeRepo(overrides: Partial<Repo> = {}): Repo {
} as Repo
}
function HookProbe(props: { repo: Repo; onRefreshReview: () => Promise<void> }): null {
type ProviderOverrides = {
review?: HostedReviewActionInfo
isGitLab?: boolean
shortLabel?: string
reviewLabel?: string
}
function HookProbe(props: {
repo: Repo
onRefreshReview: () => Promise<void>
overrides?: ProviderOverrides
}): null {
latest = useHostedReviewActions({
review,
review: props.overrides?.review ?? review,
githubPR,
repo: props.repo,
isGitLab: false,
shortLabel: 'PR',
reviewLabel: 'pull request',
isGitLab: props.overrides?.isGitLab ?? false,
shortLabel: props.overrides?.shortLabel ?? 'PR',
reviewLabel: props.overrides?.reviewLabel ?? 'pull request',
defaultMergeMethod: 'squash',
autoMergeAction: null,
onRefreshReview: props.onRefreshReview
@@ -61,12 +72,16 @@ function HookProbe(props: { repo: Repo; onRefreshReview: () => Promise<void> }):
return null
}
async function renderHook(repo: Repo, onRefreshReview = vi.fn().mockResolvedValue(undefined)) {
async function renderHook(
repo: Repo,
onRefreshReview = vi.fn().mockResolvedValue(undefined),
overrides?: ProviderOverrides
) {
const container = document.createElement('div')
document.body.appendChild(container)
root = createRoot(container)
await act(async () => {
root?.render(createElement(HookProbe, { repo, onRefreshReview }))
root?.render(createElement(HookProbe, { repo, onRefreshReview, overrides }))
})
return { onRefreshReview }
}
@@ -140,4 +155,62 @@ describe('useHostedReviewActions', () => {
expect(runtimeRpcMocks.callRuntimeRpc).not.toHaveBeenCalled()
expect(onRefreshReview).toHaveBeenCalledTimes(1)
})
it('confirms before merging with the selected strategy label (#7943)', async () => {
await renderHook(makeRepo())
await act(async () => {
await latest?.handleMerge('merge')
})
expect(confirmationMocks.confirm).toHaveBeenCalledWith(
expect.objectContaining({
title: 'Create merge commit PR #1015?',
confirmLabel: 'Create merge commit'
})
)
})
it('does not merge when the confirmation is cancelled (#7943)', async () => {
confirmationMocks.confirm.mockResolvedValue(false)
const { onRefreshReview } = await renderHook(makeRepo())
await act(async () => {
await latest?.handleMerge('squash')
})
expect(confirmationMocks.confirm).toHaveBeenCalledTimes(1)
expect(window.api.gh.mergePR).not.toHaveBeenCalled()
expect(runtimeRpcMocks.callRuntimeRpc).not.toHaveBeenCalled()
expect(onRefreshReview).not.toHaveBeenCalled()
})
it('confirms GitLab MR merges with provider-aware copy (#7943)', async () => {
await renderHook(makeRepo(), undefined, {
review: {
provider: 'gitlab',
number: 42,
state: 'open',
status: 'success',
mergeable: 'MERGEABLE'
},
isGitLab: true,
shortLabel: 'MR',
reviewLabel: 'merge request'
})
await act(async () => {
await latest?.handleMerge('squash')
})
expect(confirmationMocks.confirm).toHaveBeenCalledWith(
expect.objectContaining({ title: 'Squash and merge MR !42?' })
)
expect(window.api.gl.mergeMR).toHaveBeenCalledWith({
repoPath: '/repo',
repoId: 'repo-1',
iid: 42,
method: 'squash'
})
})
})
@@ -5,6 +5,7 @@ import type { GitHubPRAutoMergeAction } from '@/components/github-pr-merge-state
import type { HostedReviewInfo } from '../../../../shared/hosted-review'
import type { PRInfo, Repo } from '../../../../shared/types'
import type { GitHubPRMergeMethod } from '../../../../shared/types'
import { GITHUB_PR_MERGE_METHOD_LABELS } from '../../../../shared/github-pr-merge-methods'
import {
mergeGitHubHostedReview,
setGitHubHostedReviewAutoMerge,
@@ -63,6 +64,21 @@ export function useHostedReviewActions({
const handleMerge = useCallback(
async (method: GitHubPRMergeMethod = defaultMergeMethod) => {
// Why: choosing a strategy from the merge dropdown must not merge on its
// own — confirm first, matching every other PR/MR merge surface (#7943).
const label = GITHUB_PR_MERGE_METHOD_LABELS[method]
const confirmed = await confirm({
title: `${label} ${shortLabel} ${isGitLab ? '!' : '#'}${review.number}?`,
description: translate(
'auto.components.right.sidebar.use.hosted.review.actions.e475c29b17',
'This will merge the {{value0}}.',
{ value0: reviewLabel }
),
confirmLabel: label
})
if (!confirmed) {
return
}
setMerging(true)
setActionError(null)
try {
@@ -90,7 +106,17 @@ export function useHostedReviewActions({
setMerging(false)
}
},
[githubPR?.prRepo, isGitLab, defaultMergeMethod, onRefreshReview, repo, review.number]
[
confirm,
githubPR?.prRepo,
isGitLab,
shortLabel,
reviewLabel,
defaultMergeMethod,
onRefreshReview,
repo,
review.number
]
)
const handleAutoMerge = useCallback(async () => {
+7
View File
@@ -10098,6 +10098,13 @@
"cfafa92509": "No unresolved conflicts to send."
}
}
},
"hosted": {
"review": {
"actions": {
"e475c29b17": "This will merge the {{value0}}."
}
}
}
},
"fileExplorerOperationOwner": {
+7
View File
@@ -10075,6 +10075,13 @@
"cfafa92509": "No hay conflictos sin resolver para enviar."
}
}
},
"hosted": {
"review": {
"actions": {
"e475c29b17": "This will merge the {{value0}}."
}
}
}
},
"fileExplorerOperationOwner": {
+7
View File
@@ -10075,6 +10075,13 @@
"cfafa92509": "送信する未解決の競合はありません。"
}
}
},
"hosted": {
"review": {
"actions": {
"e475c29b17": "This will merge the {{value0}}."
}
}
}
},
"fileExplorerOperationOwner": {
+7
View File
@@ -10075,6 +10075,13 @@
"cfafa92509": "보낼 해결되지 않은 충돌이 없습니다."
}
}
},
"hosted": {
"review": {
"actions": {
"e475c29b17": "This will merge the {{value0}}."
}
}
}
},
"fileExplorerOperationOwner": {
+7
View File
@@ -10075,6 +10075,13 @@
"cfafa92509": "没有未解决的冲突需要发送。"
}
}
},
"hosted": {
"review": {
"actions": {
"e475c29b17": "This will merge the {{value0}}."
}
}
}
},
"fileExplorerOperationOwner": {