Skip to content
Open
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 @@ -150,6 +150,7 @@ import {
pullRequestCheckoutCommand,
pullRequestFindingKey,
pullRequestHandoffLabels,
mergeRefusalHint,
PULL_REQUEST_MERGE_METHOD_LABELS,
readableFailure,
readPullRequestDetailSnapshot,
Expand Down Expand Up @@ -988,7 +989,13 @@ export function PullRequestDetailPanel({
const hint =
updateMethod === "rebase"
? UPDATE_BRANCH_REBASE_FAILURE_HINT
: ACTION_FAILURE_HINTS[action];
: action === "merge" && detail !== null
? (mergeRefusalHint({
mergeability: detail.mergeability,
reviewDecision: sharedSummary?.reviewDecision,
checksState,
}) ?? ACTION_FAILURE_HINTS.merge)
: ACTION_FAILURE_HINTS[action];
toastManager.add({
type: "error",
title: ACTION_FAILURE_LABELS[action],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
pullRequestCheckoutCommand,
pullRequestFindingKey,
pullRequestReviewOutcome,
mergeRefusalHint,
readableFailure,
readPullRequestDetailSnapshot,
resolveDisplayedPullRequestDetail,
Expand Down Expand Up @@ -1045,6 +1046,47 @@ describe("what to say when an action fails", () => {
});
});

describe("why a merge was refused", () => {
const clear = {
mergeability: "mergeable",
reviewDecision: null,
checksState: "passing",
} as const;

it("names a required review, the blocker the host only calls a policy", () => {
expect(mergeRefusalHint({ ...clear, reviewDecision: "review-required" })).toContain(
"approving review",
);
});

it("puts conflicts ahead of reviews and checks, since they need fixing first", () => {
expect(
mergeRefusalHint({
mergeability: "conflicting",
reviewDecision: "review-required",
checksState: "failing",
}),
).toContain("conflicts");
});

it("names failing or running checks when reviews are not in the way", () => {
expect(mergeRefusalHint({ ...clear, checksState: "failing" })).toContain("failing");
expect(mergeRefusalHint({ ...clear, checksState: "pending" })).toContain("still running");
});

it("blames the branch rules when nothing visible explains the refusal", () => {
// The author's own approval shows as "approved" but satisfies no rule.
expect(mergeRefusalHint({ ...clear, reviewDecision: "approved" })).toContain("branch rules");
expect(mergeRefusalHint(clear)).toContain("branch rules");
});

it("leaves the generic hint when the host has not said whether it can merge", () => {
expect(
mergeRefusalHint({ mergeability: "unknown", reviewDecision: undefined, checksState: null }),
).toBeNull();
});
});

describe("findings that are already on a line", () => {
it("does not quote a resolved thread's comment as a remark with nowhere to hang", () => {
const resolved: PullRequestReviewThread = {
Expand Down
38 changes: 38 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestDetail.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
type PullRequestReaction,
type PullRequestRef,
type RepositoryIdentity,
type PullRequestReviewDecision,
type PullRequestReviewThread,
type PullRequestState,
type PullRequestUpdateMethod,
Expand Down Expand Up @@ -1075,6 +1076,43 @@ export function readableFailure(failure: unknown, hint: string): string {
return bounded;
}

/**
* What to say under a refused merge when the host gave no reason. `gh` reports a blocked merge
* only as "the base branch policy prohibits the merge", so this names the blocker from what the
* page already knows. Null when mergeability is unknown, which leaves the generic hint.
*/
export function mergeRefusalHint(state: {
readonly mergeability: PullRequestMergeability;
readonly reviewDecision: PullRequestReviewDecision | null | undefined;
readonly checksState: PullRequestChecksState | null;
}): string | null {
if (state.mergeability === "unknown") return null;
if (state.mergeability === "conflicting") {
Comment thread
TonybynMp4 marked this conversation as resolved.
return "The branch conflicts with the base. Resolve the conflicts, then merge again.";
}
// GitHub only reports a missing review when a branch rule requires one. The reverse does not
// hold: the server shows "approved" once any approval exists, including the author's own, which
// no rule counts.
if (state.reviewDecision === "review-required") {
return "The branch rules require an approving review before this can merge.";
}
if (state.reviewDecision === "changes-requested") {
return "A reviewer requested changes. Their review has to be addressed before this can merge.";
}
if (state.checksState === "failing") {
return "Some checks are failing, and the branch rules may require them to pass.";
}
if (state.checksState === "pending") {
return "Some checks are still running or awaiting action, and the branch rules may require them to pass.";
}
// Mergeable with nothing failing, yet refused: a branch rule the page cannot see, such as an
// approval from someone other than the author, signed commits, or resolved conversations.
if (state.mergeability === "mergeable") {
return "The branch rules blocked the merge. They may need an approval from someone other than the author, signed commits, or resolved conversations. The pull request on the host lists what is missing.";
}
return null;
}

/**
* Where the branch stands against its base, said the way GitHub says it: current, out of date but
* still cleanly mergeable, or conflicting. Only the middle one is an offer — the conflicts row
Expand Down
Loading