Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -104,6 +104,7 @@ import {
PullRequestStatusMenu,
PullRequestChecksRow,
PullRequestBranchRow,
PullRequestDeleteBranchRow,
PullRequestReviewersRow,
} from "./PullRequestStatusRows";
import { PullRequestMergeControls } from "./PullRequestMergeControls";
Expand Down Expand Up @@ -249,6 +250,7 @@ function PullRequestOverview(props: {
<Animated.View layout={PR_ROW_LAYOUT}>
<PullRequestMergeControls pr={pr} params={props.params} stack={props.stack} />
</Animated.View>
<PullRequestDeleteBranchRow pr={pr} params={props.params} />
</PullRequestCardGroup>
{pr.labels.length ? (
<Animated.View layout={PR_ROW_LAYOUT} className="flex-row flex-wrap gap-2">
Expand Down
41 changes: 40 additions & 1 deletion apps/mobile/src/features/pull-requests/PullRequestStatusRows.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ import { PullRequestCardRow } from "./PullRequestControls";
import { PullRequestAgentRow } from "./PullRequestAgentRow";
import { composerPullRequests } from "../../state/pull-requests";
import { useEnvironmentQuery } from "../../state/query";
import { allowedUpdateMethods, PR_ACTION_ICONS, PR_ACTION_LABELS } from "./pullRequestActions";
import {
allowedUpdateMethods,
canPerformPullRequestAction,
PR_ACTION_ICONS,
PR_ACTION_LABELS,
} from "./pullRequestActions";
import {
checkStatus,
pullRequestBranchSummary,
Expand All @@ -25,6 +30,7 @@ import {
import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff";
import { usePullRequestAction } from "./usePullRequestAction";
import { usePullRequestAgentActions } from "./usePullRequestAgentActions";
import { usePullRequestPermissions } from "./usePullRequestPermissions";

type RowProps = { readonly pr: PullRequestDetail; readonly params: PullRequestParams };
const STATUS_ACTIONS: readonly PullRequestAction[] = [
Expand Down Expand Up @@ -226,6 +232,39 @@ export function PullRequestBranchRow(props: RowProps) {
);
}

export function PullRequestDeleteBranchRow(props: RowProps) {
const action = usePullRequestAction(props.params, props.pr);
const { connected } = usePullRequestPermissions(props.params, props.pr);
if (!canPerformPullRequestAction(props.pr, "delete-branch")) return null;
const repository = props.pr.headRepositoryNameWithOwner ?? props.pr.repository;
return (
<Animated.View
layout={PR_ROW_LAYOUT}
exiting={PR_ROW_EXIT}
className="border-t border-border-subtle"
>
<PullRequestCardRow
title={PR_ACTION_LABELS["delete-branch"]}
icon={PR_ACTION_ICONS["delete-branch"]}
destructive={connected}
value={action.busy ? "Deleting…" : undefined}
detail={connected ? props.pr.headBranch : "Reconnect to delete this branch."}
last
onPress={
action.available("delete-branch")
? () =>
action.confirm(
"delete-branch",
PR_ACTION_LABELS["delete-branch"],
`Delete ${props.pr.headBranch} from ${repository}? Local branches and worktrees are not changed.`,
)
: undefined
}
/>
</Animated.View>
);
}

export function PullRequestReviewersRow(props: RowProps) {
const [expanded, setExpanded] = useState(false);
const navigation = useNavigation();
Expand Down
13 changes: 13 additions & 0 deletions apps/mobile/src/features/pull-requests/pullRequestActions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,19 @@ describe("mobile pull request actions", () => {
expect(mergeSummary(unknown, true)).toBe("Squash");
});

it("offers deleting the source branch only once merged or closed on a host that supports it", () => {
const capabilities = { ...detail().capabilities, deleteBranch: true };
expect(canPerformPullRequestAction(detail({ capabilities }), "delete-branch")).toBe(false);
for (const state of ["merged", "closed"] as const) {
expect(canPerformPullRequestAction(detail({ state, capabilities }), "delete-branch")).toBe(
true,
);
expect(canPerformPullRequestAction(detail({ state }), "delete-branch")).toBe(false);
}
const refused = detail({ state: "merged", capabilities, viewerPermissions: { actions: [] } });
expect(canPerformPullRequestAction(refused, "delete-branch")).toBe(false);
});

it("cannot merge when the repository has disabled every offered strategy", () => {
const pr = detail({ mergeCapabilities: { merge: false, squash: false, rebase: false } });
expect(canPerformPullRequestAction(pr, "merge")).toBe(false);
Expand Down
7 changes: 6 additions & 1 deletion apps/mobile/src/features/pull-requests/pullRequestActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type ActionDetail = Pick<
> & {
readonly capabilities: Pick<
PullRequestDetail["capabilities"],
"actions" | "mergeMethods" | "updateMethods"
"actions" | "mergeMethods" | "updateMethods" | "deleteBranch"
>;
readonly viewerPermissions: Pick<
PullRequestDetail["viewerPermissions"],
Expand Down Expand Up @@ -41,6 +41,7 @@ export const PR_ACTION_LABELS = {
"enable-auto-merge": "Enable auto-merge",
"disable-auto-merge": "Disable auto-merge",
revert: "Create revert pull request",
"delete-branch": "Delete branch",
"approve-workflows": "Approve workflows",
} as const satisfies Record<PullRequestAction, string>;

Expand All @@ -54,6 +55,7 @@ export const PR_ACTION_ICONS = {
"enable-auto-merge": "clock",
"disable-auto-merge": "xmark",
revert: "arrow.uturn.backward",
"delete-branch": "trash",
"approve-workflows": "checkmark",
} as const satisfies Record<PullRequestAction, AppSymbolName>;

Expand All @@ -68,6 +70,7 @@ export const ALL_PR_ACTIONS: readonly PullRequestAction[] = [
"close",
"reopen",
"revert",
"delete-branch",
];

export function allowedMergeMethods(pr: ActionDetail) {
Expand All @@ -86,6 +89,8 @@ export function canPerformPullRequestAction(pr: ActionDetail, action: PullReques
return false;
if (action === "reopen") return pr.state === "closed";
if (action === "revert") return pr.state === "merged";
if (action === "delete-branch")
return pr.state !== "open" && pr.capabilities.deleteBranch === true;
if (pr.state !== "open") return false;
switch (action) {
case "update-branch":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export function usePullRequestAction(params: PullRequestParams, pr: PullRequestD
{ text: "Cancel", style: "cancel" },
{
text: title,
style: action === "close" ? "destructive" : "default",
style: action === "close" || action === "delete-branch" ? "destructive" : "default",
onPress: () => void perform(action),
},
]);
Expand Down
70 changes: 70 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1573,6 +1573,76 @@ it.effect("refreshes every reader before a queued merge confirmation finishes",
),
);

it.effect("runs branch deletion only when the host and viewer support it, never for a stack", () =>
Effect.gen(function* () {
let actions = 0;
let supported = false;
let allowed = true;
const base = fakeProvider(SourceControlProviderKind.make("github"));
const service = yield* makeService({
projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
providers: [
{
...base,
capabilities: {
...base.capabilities,
actions: ["delete-branch"],
stackActions: true,
get deleteBranch() {
return supported;
},
},
getViewerPermissions: () =>
Effect.succeed({
actions: allowed ? ["delete-branch"] : [],
comment: true,
resolve: true,
verdicts: [],
requestReviewers: false,
}),
runAction: (input) =>
Effect.sync(() => {
assert.strictEqual(input.action, "delete-branch");
actions++;
}),
},
],
});
const input = {
projectId: "p1" as ProjectId,
repository: "acme/web",
number: 1,
action: "delete-branch" as const,
};
assert.strictEqual(
(yield* Effect.flip(service.runAction(input)))._tag,
"PullRequestOperationError",
);
assert.strictEqual(actions, 0);
supported = true;
assert.strictEqual(
(yield* Effect.flip(
service.runAction({
...input,
stackNumber: 2,
expectedStackHeads: [{ number: 1, headSha: "abc123" }],
}),
))._tag,
"PullRequestOperationError",
);
assert.strictEqual(actions, 0);
allowed = false;
assert.strictEqual(
(yield* Effect.flip(service.runAction(input)))._tag,
"PullRequestOperationError",
);
assert.strictEqual(actions, 0);
allowed = true;
yield* service.runAction(input);
assert.strictEqual(actions, 1);
}),
);

it.effect("refuses an action this viewer may not take, and says what access it takes", () =>
Effect.gen(function* () {
let ran: string | null = null;
Expand Down
9 changes: 9 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ const ACTION_ACCESS_REFUSALS: Record<PullRequestAction, string> = {
"disable-auto-merge":
"You need write access on this repository to stop it being merged for you once it is ready.",
revert: "You need write access on this repository to open a revert pull request.",
"delete-branch": "You need write access on the source repository to delete its branch.",
"approve-workflows":
"You need write access on this repository to approve workflows from a fork pull request.",
};
Expand Down Expand Up @@ -2019,6 +2020,14 @@ export const make = Effect.gen(function* () {
const runAction = (input: PullRequestActionInput): Effect.Effect<string, PullRequestError> =>
requireProject(input).pipe(
Effect.flatMap((project): Effect.Effect<string, PullRequestError> => {
if (input.action === "delete-branch" && project.api.capabilities.deleteBranch !== true) {
return Effect.fail(
new PullRequestOperationError({
operation: "runAction",
detail: "This host cannot delete a change request's source branch.",
}),
);
}
if (
input.stackNumber !== undefined &&
(project.api.capabilities.stackActions !== true ||
Expand Down
55 changes: 55 additions & 0 deletions apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
PlayIcon,
RotateCcwIcon,
SparklesIcon,
Trash2Icon,
TriangleAlertIcon,
} from "lucide-react";
import {
Expand Down Expand Up @@ -161,6 +162,7 @@ import {
resolveDisplayedPullRequestDetail,
resolvePullRequestPrimaryControl,
allowsSinglePullRequestMerge,
canDeletePullRequestBranch,
resolveBaseFreshness,
resolvePullRequestMergeMethod,
type PullRequestFinding,
Expand Down Expand Up @@ -204,6 +206,7 @@ const ACTION_FAILURE_LABELS: Record<PullRequestAction, string> = {
"enable-auto-merge": "Could not turn on auto-merge",
"disable-auto-merge": "Could not turn off auto-merge",
revert: "Could not open a revert pull request",
"delete-branch": "Could not delete this branch",
"approve-workflows": "Could not approve workflows",
};

Expand All @@ -228,6 +231,8 @@ const ACTION_FAILURE_HINTS: Record<PullRequestAction, string> = {
"The host refused it. Check that you have write access, and that the merge has not already happened.",
revert:
"The host refused it. Check that you have write access and that this pull request was merged on the host.",
"delete-branch":
"The host refused it. Check that you have write access to the source repository and that the branch has not moved since this page loaded.",
"approve-workflows":
"The host refused it. Check that you have Actions write access and that these workflow runs are still awaiting approval.",
};
Expand Down Expand Up @@ -1030,6 +1035,9 @@ export function PullRequestDetailPanel({
if (action === "revert") {
toastManager.add({ type: "success", title: "Revert pull request opened" });
}
if (action === "delete-branch") {
toastManager.add({ type: "success", title: "Branch deleted" });
}
// A branch update moves the head commit, which leaves the diff atom pointed at a comparison
// that no longer exists — the same staleness the manual refresh button fixes, so it goes
// through that path rather than a second one. Every other action here only changes metadata;
Expand Down Expand Up @@ -1639,6 +1647,22 @@ export function PullRequestDetailPanel({
const statePresentation = detail
? resolvePullRequestState({ state: detail.state, isDraft: detail.isDraft })
: null;
const showsDeleteBranch = detail !== null && canDeletePullRequestBranch(detail);
const environmentConnected = environments.some(
(environment) =>
environment.environmentId === environmentId && environment.connection.phase === "connected",
);
const confirmDeleteBranch = async () => {
if (detail === null) return;
const confirmed = await readLocalApi()?.dialogs.confirm(
[
`Delete branch ${detail.headBranch}?`,
`This deletes ${detail.headBranch} from ${detail.headRepositoryNameWithOwner ?? detail.repository}. Local branches and worktrees are not changed.`,
].join("\n"),
{ variant: "destructive" },
);
if (confirmed) void perform("delete-branch");
};
const showsApproveWorkflows =
workflowApprovalsRequired > 0 && !checksStale && can("approve-workflows");
const checksSummary = checksStale
Expand Down Expand Up @@ -2157,6 +2181,37 @@ export function PullRequestDetailPanel({
</span>
</Badge>
) : null}
{showsDeleteBranch ? (
<Tooltip>
<TooltipTrigger
render={
<span className="inline-flex shrink-0">
<Button
size="xs"
variant="destructive-outline"
disabled={actionPending || !environmentConnected}
onClick={() => void confirmDeleteBranch()}
aria-label={
pendingAction === "delete-branch"
? "Deleting branch..."
: "Delete branch"
}
>
<Trash2Icon aria-hidden className="size-3.5" />
<span className="@max-[30rem]/pr-header:hidden">
{pendingAction === "delete-branch" ? "Deleting..." : "Delete branch"}
</span>
</Button>
</span>
}
/>
<TooltipPopup side="top">
{environmentConnected
? `Delete ${detail.headBranch}`
: "Reconnect to delete this branch"}
</TooltipPopup>
</Tooltip>
) : null}
{/* Keep checkout beside the menu so host actions arriving on its left cannot
move a different action under the reader's pointer. */}
{checkoutControl}
Expand Down
Loading
Loading