Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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();
});
});
21 changes: 18 additions & 3 deletions apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -94,6 +95,7 @@ import {
} from "./pullRequestPresentation";
import {
PULL_REQUEST_MERGE_METHOD_LABELS,
PULL_REQUEST_MERGE_METHOD_WORDS,
appendHandoffToDraft,
buildReviewCommentHandoff,
formatPullRequestBaseFreshness,
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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";

Expand Down Expand Up @@ -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]}
</Button>
{detail.mergeMethods.length > 1 ? (
<Menu>
Expand Down Expand Up @@ -1329,7 +1343,7 @@ function usePullRequestActions({
setConfirming({ action: "merge", mergeMethod: defaultMergeMethod })
}
>
Merge
{PULL_REQUEST_MERGE_METHOD_LABELS[defaultMergeMethod]}
</MenuItem>
) : null}
{handoffs ? (
Expand Down Expand Up @@ -1456,6 +1470,7 @@ function usePullRequestActions({
onClick={() => {
setConfirming(null);
if (confirming.action === "merge") {
setRememberedMergeMethod(confirming.mergeMethod);
run("merge", {
mergeMethod: confirming.mergeMethod,
...(deleteBranch ? { deleteBranch: true } : {}),
Expand Down
13 changes: 13 additions & 0 deletions apps/web/src/components/pull-requests/pullRequests.logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
pullRequestFiltersFromSearch,
pullRequestFiltersToSearch,
pullRequestLabelColor,
resolveDefaultMergeMethod,
resolveNeedsYouReason,
resolvePullRequestMergeBlock,
resolvePullRequestReviewPosition,
Expand Down Expand Up @@ -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");
});
});
21 changes: 17 additions & 4 deletions apps/web/src/components/pull-requests/pullRequests.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1328,15 +1328,28 @@ export const PULL_REQUEST_MERGE_METHOD_LABELS: Readonly<Record<PullRequestMergeM
rebase: "Rebase and merge",
};

/** The one word the Merge button itself wears for each method. */
export const PULL_REQUEST_MERGE_METHOD_WORDS: Readonly<Record<PullRequestMergeMethod, string>> = {
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";
}

Expand Down
Loading