From a7e10c9b47e54edd0ebd6656a203399c96584290 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:05:41 -0400 Subject: [PATCH] feat(web): the Merge button remembers the method last run on a repository The Merge button always ran the repository's first allowed method, so anyone who squashes had to open the arrow menu every time, while GitHub's own site remembers the last choice. The method a merge is confirmed with is now stored per repository on this computer and becomes the button's own while the repository still allows it. The button wears the method as one word (Merge, Squash, Rebase); the arrow menu and the confirm dialog keep the full names. Covered by a logic test and a browser test that merges, reopens, and finds the remembered method on the button. --- .../PullRequestDetailPanel.browser.tsx | 33 +++++++++++++++++++ .../pull-requests/PullRequestDetailPanel.tsx | 21 ++++++++++-- .../pull-requests/pullRequests.logic.test.ts | 13 ++++++++ .../pull-requests/pullRequests.logic.ts | 21 +++++++++--- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx index 5557dc38..627528bd 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx @@ -261,6 +261,9 @@ describe("PullRequestDetailPanel", () => { // The merge dialog remembers this per computer, so one test's tick would // otherwise be the next one's default. window.localStorage.removeItem(`threadlines:pull-requests:delete-branch:v1:${ENVIRONMENT_ID}`); + window.localStorage.removeItem( + "threadlines:pull-requests:merge-method:v1:github:threadlines/threadlines", + ); }); it("renders the header, the checks that need attention, and the conversation", async () => { @@ -693,4 +696,34 @@ describe("PullRequestDetailPanel", () => { await rendered.cleanup(); }); + + it("wears the method last run on the repository and runs it next time", async () => { + const viewer = { canWrite: true, canReview: false, canManage: true }; + const first = await renderPanel({ detail: { viewer } }); + + // The repository lists squash first, so that is the button's own until a + // choice is made. The menu spells the methods out in full. + await expect.element(page.getByTestId("pull-request-merge")).toHaveTextContent("Squash"); + await userEvent.click(page.getByRole("button", { name: "Choose a merge method" })); + await userEvent.click(page.getByRole("menuitem", { name: "Create a merge commit" })); + const dialog = page.getByRole("alertdialog"); + await expect.element(dialog.getByText("Create a merge commit.")).toBeVisible(); + await userEvent.click(dialog.getByRole("button", { name: "Merge" })); + await vi.waitFor(() => { + expect(first.runAction).toHaveBeenCalledWith({ + ...REFERENCE, + action: "merge", + mergeMethod: "merge", + }); + }); + await first.cleanup(); + + const second = await renderPanel({ detail: { viewer } }); + await expect.element(page.getByTestId("pull-request-merge")).toHaveTextContent("Merge"); + await userEvent.click(page.getByTestId("pull-request-merge")); + await expect + .element(page.getByRole("alertdialog").getByText("Create a merge commit.")) + .toBeVisible(); + await second.cleanup(); + }); }); diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx index e0790a86..d78e8b9d 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx @@ -12,6 +12,7 @@ import type { ScopedThreadRef, SourceControlProviderKind, } from "@threadlines/contracts"; +import { PullRequestMergeMethod as PullRequestMergeMethodSchema } from "@threadlines/contracts"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { useNavigate } from "@tanstack/react-router"; import * as Schema from "effect/Schema"; @@ -94,6 +95,7 @@ import { } from "./pullRequestPresentation"; import { PULL_REQUEST_MERGE_METHOD_LABELS, + PULL_REQUEST_MERGE_METHOD_WORDS, appendHandoffToDraft, buildReviewCommentHandoff, formatPullRequestBaseFreshness, @@ -1061,6 +1063,11 @@ type PullRequestConfirmation = /** Remembered per computer: whoever deletes merged branches always does. */ const DELETE_BRANCH_STORAGE_PREFIX = "threadlines:pull-requests:delete-branch:v1"; +/** Remembered per repository: the merge method last run on it becomes the + * Merge button's own, as the host's site does. */ +const MERGE_METHOD_STORAGE_PREFIX = "threadlines:pull-requests:merge-method:v1"; +const REMEMBERED_MERGE_METHOD_SCHEMA = Schema.NullOr(PullRequestMergeMethodSchema); + /** The three pieces the header hangs in three different places. */ interface PullRequestActionsView { /** The buttons, for the right of the header's first row. */ @@ -1104,6 +1111,11 @@ function usePullRequestActions({ false, Schema.Boolean, ); + const [rememberedMergeMethod, setRememberedMergeMethod] = useLocalStorage( + `${MERGE_METHOD_STORAGE_PREFIX}:${detail.provider}:${detail.repository}`, + null, + REMEMBERED_MERGE_METHOD_SCHEMA, + ); const isRunning = mutation.isPending; const runningAction = isRunning ? (mutation.variables?.action ?? null) : null; @@ -1144,7 +1156,7 @@ function usePullRequestActions({ const updateMethods = detail.capabilities.updateMethods; const mergeBlock = resolvePullRequestMergeBlock(detail); const mergeDisabled = isRunning || mergeBlock !== null; - const defaultMergeMethod = resolveDefaultMergeMethod(detail.mergeMethods); + const defaultMergeMethod = resolveDefaultMergeMethod(detail.mergeMethods, rememberedMergeMethod); const canUpdateBranch = canWrite && isOpen && allows("update-branch"); const isBehind = detail.baseComparison === "behind"; @@ -1180,7 +1192,9 @@ function usePullRequestActions({ data-testid="pull-request-merge" onClick={() => setConfirming({ action: "merge", mergeMethod: defaultMergeMethod })} > - {runningAction === "merge" ? RUNNING_ACTION_WORDS.merge : "Merge"} + {runningAction === "merge" + ? RUNNING_ACTION_WORDS.merge + : PULL_REQUEST_MERGE_METHOD_WORDS[defaultMergeMethod]} {detail.mergeMethods.length > 1 ? ( @@ -1329,7 +1343,7 @@ function usePullRequestActions({ setConfirming({ action: "merge", mergeMethod: defaultMergeMethod }) } > - Merge + {PULL_REQUEST_MERGE_METHOD_LABELS[defaultMergeMethod]} ) : null} {handoffs ? ( @@ -1456,6 +1470,7 @@ function usePullRequestActions({ onClick={() => { setConfirming(null); if (confirming.action === "merge") { + setRememberedMergeMethod(confirming.mergeMethod); run("merge", { mergeMethod: confirming.mergeMethod, ...(deleteBranch ? { deleteBranch: true } : {}), diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 7a19ad01..d6df2f26 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -37,6 +37,7 @@ import { pullRequestFiltersFromSearch, pullRequestFiltersToSearch, pullRequestLabelColor, + resolveDefaultMergeMethod, resolveNeedsYouReason, resolvePullRequestMergeBlock, resolvePullRequestReviewPosition, @@ -1221,3 +1222,15 @@ describe("applyPendingPullRequestReactions", () => { ).toEqual([{ content: "thumbs-up", count: 3, viewerReacted: true }]); }); }); + +describe("resolveDefaultMergeMethod", () => { + it("runs the method last used on the repository while it is still allowed", () => { + expect(resolveDefaultMergeMethod(["merge", "squash", "rebase"], "squash")).toBe("squash"); + }); + + it("falls back to the repository's first method when nothing is remembered or it is off", () => { + expect(resolveDefaultMergeMethod(["squash", "rebase"], "merge")).toBe("squash"); + expect(resolveDefaultMergeMethod(["squash", "rebase"])).toBe("squash"); + expect(resolveDefaultMergeMethod([], "squash")).toBe("merge"); + }); +}); diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 00c07ab4..a576758f 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -1328,15 +1328,28 @@ export const PULL_REQUEST_MERGE_METHOD_LABELS: Readonly> = { + merge: "Merge", + squash: "Squash", + rebase: "Rebase", +}; + /** - * The method the Merge button runs without being asked. The repository lists - * what it allows in its own order and the first one is its default; a host - * that reports nothing still gets a plain merge offered, and refuses it itself - * if it really is off. + * The method the Merge button runs without being asked: the one last used on + * this repository while the repository still allows it, the way the host's own + * site remembers a choice. Before any choice, the repository lists what it + * allows in its own order and the first one is its default; a host that reports + * nothing still gets a plain merge offered, and refuses it itself if it really + * is off. */ export function resolveDefaultMergeMethod( mergeMethods: readonly PullRequestMergeMethod[], + remembered: PullRequestMergeMethod | null = null, ): PullRequestMergeMethod { + if (remembered !== null && mergeMethods.includes(remembered)) { + return remembered; + } return mergeMethods[0] ?? "merge"; }