From 37cd830192309267e224bbbaf413a22d73de8f37 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 19 Jul 2026 13:09:36 -0700 Subject: [PATCH] Confirm before merging a PR/MR from the sidebar strategy dropdown (#8911) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../use-hosted-review-actions.test.tsx | 87 +++++++++++++++++-- .../use-hosted-review-actions.ts | 28 +++++- src/renderer/src/i18n/locales/en.json | 7 ++ src/renderer/src/i18n/locales/es.json | 7 ++ src/renderer/src/i18n/locales/ja.json | 7 ++ src/renderer/src/i18n/locales/ko.json | 7 ++ src/renderer/src/i18n/locales/zh.json | 7 ++ 7 files changed, 142 insertions(+), 8 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/use-hosted-review-actions.test.tsx b/src/renderer/src/components/right-sidebar/use-hosted-review-actions.test.tsx index 82527dc8ed5..761abf269f4 100644 --- a/src/renderer/src/components/right-sidebar/use-hosted-review-actions.test.tsx +++ b/src/renderer/src/components/right-sidebar/use-hosted-review-actions.test.tsx @@ -46,14 +46,25 @@ function makeRepo(overrides: Partial = {}): Repo { } as Repo } -function HookProbe(props: { repo: Repo; onRefreshReview: () => Promise }): null { +type ProviderOverrides = { + review?: HostedReviewActionInfo + isGitLab?: boolean + shortLabel?: string + reviewLabel?: string +} + +function HookProbe(props: { + repo: Repo + onRefreshReview: () => Promise + 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 }): 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' + }) + }) }) diff --git a/src/renderer/src/components/right-sidebar/use-hosted-review-actions.ts b/src/renderer/src/components/right-sidebar/use-hosted-review-actions.ts index ed3a1b23bcc..7f2274bfdcc 100644 --- a/src/renderer/src/components/right-sidebar/use-hosted-review-actions.ts +++ b/src/renderer/src/components/right-sidebar/use-hosted-review-actions.ts @@ -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 () => { diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 5d48f72c149..9bad363c2b3 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -10098,6 +10098,13 @@ "cfafa92509": "No unresolved conflicts to send." } } + }, + "hosted": { + "review": { + "actions": { + "e475c29b17": "This will merge the {{value0}}." + } + } } }, "fileExplorerOperationOwner": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 163138a3f18..05b76df3900 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -10075,6 +10075,13 @@ "cfafa92509": "No hay conflictos sin resolver para enviar." } } + }, + "hosted": { + "review": { + "actions": { + "e475c29b17": "This will merge the {{value0}}." + } + } } }, "fileExplorerOperationOwner": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 9bcc9051304..8150d25ca97 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -10075,6 +10075,13 @@ "cfafa92509": "送信する未解決の競合はありません。" } } + }, + "hosted": { + "review": { + "actions": { + "e475c29b17": "This will merge the {{value0}}." + } + } } }, "fileExplorerOperationOwner": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 73b1d12b378..cc28a9e9dd9 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -10075,6 +10075,13 @@ "cfafa92509": "보낼 해결되지 않은 충돌이 없습니다." } } + }, + "hosted": { + "review": { + "actions": { + "e475c29b17": "This will merge the {{value0}}." + } + } } }, "fileExplorerOperationOwner": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 874022be095..a649bc1611c 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -10075,6 +10075,13 @@ "cfafa92509": "没有未解决的冲突需要发送。" } } + }, + "hosted": { + "review": { + "actions": { + "e475c29b17": "This will merge the {{value0}}." + } + } } }, "fileExplorerOperationOwner": {