authorgravatar for 62337003+Bil0000@users.noreply.github.comBilal Bakr <62337003+Bil0000@users.noreply.github.com> 2026-08-15 05:48:54+03:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-08-14 22:48:54-04:00
log8c628f14993cb159d467e7a0f8c52578dde77005
tree06b6d2cf8135718539bc99ff4054fe810b3e0baa
parent48ddb3d469d245363dba723f774ba449e4d950c5
signature Signed by PGP key B5690EEEBB952194

fix(web): align pull request action menu rows (#6534)

Co-authored-by: Nickolas Kyryliuk <nickolaskyryliuk@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

5 files changed, 74 insertions(+), 9 deletions(-)

apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx+34-8
...@@ -103,6 +103,7 @@ import {...@@ -103,6 +103,7 @@ import {
103 handoffPrompt,103 handoffPrompt,
104 handoffReviewComments,104 handoffReviewComments,
105 pullRequestActionNeedsHostRefresh,105 pullRequestActionNeedsHostRefresh,
106 pullRequestActionMenuHasGroup,
106 pullRequestFindingKey,107 pullRequestFindingKey,
107 pullRequestHandoffLabels,108 pullRequestHandoffLabels,
108 readableFailure,109 readableFailure,
...@@ -1033,6 +1034,26 @@ export function PullRequestDetailPanel({...@@ -1033,6 +1034,26 @@ export function PullRequestDetailPanel({
1033 : allowedMergeMethods.length > 01034 : allowedMergeMethods.length > 0
1034 ? "merge"1035 ? "merge"
1035 : null;1036 : null;
1037 // What the menu's action group holds. Named once so the separators around it are drawn from
1038 // the same answer as its contents, rather than on the assumption that it has any.
1039 const showsDraftToggle =
1040 detail?.state === "open" &&
1041 can(detail.isDraft ? "ready" : "draft") &&
1042 !(detail.isDraft && primaryAction === "ready");
1043 const showsAutoMerge =
1044 detail?.state === "open" &&
1045 ((autoMergeArmed && can("disable-auto-merge")) ||
1046 (!autoMergeArmed &&
1047 !detail.isDraft &&
1048 !conflicting &&
1049 can("enable-auto-merge") &&
1050 allowedMergeMethods.length > 0));
1051 const showsMergeMethods =
1052 detail?.state === "open" &&
1053 can("merge") &&
1054 !detail.isDraft &&
1055 !conflicting &&
1056 allowedMergeMethods.length > 1;
1036 // The pull request number carries this state in the overview and the right-panel tab mirrors1057 // The pull request number carries this state in the overview and the right-panel tab mirrors
1037 // it. Conflicts keep their own row below: an open pull request remains green there.1058 // it. Conflicts keep their own row below: an open pull request remains green there.
1038 const statePresentation = detail1059 const statePresentation = detail
...@@ -1191,8 +1212,7 @@ export function PullRequestDetailPanel({...@@ -1191,8 +1212,7 @@ export function PullRequestDetailPanel({
1191 {/* Only where the button row could not take it: "Ready for review" on a1212 {/* Only where the button row could not take it: "Ready for review" on a
1192 draft is the primary header button, so offering it here as well would1213 draft is the primary header button, so offering it here as well would
1193 show the same action twice. */}1214 show the same action twice. */}
1194 {can(detail.isDraft ? "ready" : "draft") &&1215 {showsDraftToggle ? (
1195 !(detail.isDraft && primaryAction === "ready") ? (
1196 <MenuItem1216 <MenuItem
1197 disabled={actionPending}1217 disabled={actionPending}
1198 onClick={() => void perform(detail.isDraft ? "ready" : "draft")}1218 onClick={() => void perform(detail.isDraft ? "ready" : "draft")}
...@@ -1236,12 +1256,12 @@ export function PullRequestDetailPanel({...@@ -1236,12 +1256,12 @@ export function PullRequestDetailPanel({
1236 Hidden while conflicting: every method would fail. */}1256 Hidden while conflicting: every method would fail. */}
1237 {/* Only where merging is on offer at all: a strategy to merge with is not1257 {/* Only where merging is on offer at all: a strategy to merge with is not
1238 a choice for someone who may not merge. */}1258 a choice for someone who may not merge. */}
1239 {can("merge") &&1259 {showsMergeMethods ? (
1240 !detail.isDraft &&
1241 !conflicting &&
1242 allowedMergeMethods.length > 1 ? (
1243 <>1260 <>
1244 <MenuSeparator />1261 {/* Only below the draft control. A host with no draft of its own, or
1262 a draft whose control is already the header button, would leave
1263 this against the separator that opened the group. */}
1264 {showsDraftToggle ? <MenuSeparator /> : null}
1245 <MenuRadioGroup1265 <MenuRadioGroup
1246 value={selectedMergeMethod}1266 value={selectedMergeMethod}
1247 onValueChange={(method) =>1267 onValueChange={(method) =>
...@@ -1261,7 +1281,13 @@ export function PullRequestDetailPanel({...@@ -1261,7 +1281,13 @@ export function PullRequestDetailPanel({
1261 </MenuRadioGroup>1281 </MenuRadioGroup>
1262 </>1282 </>
1263 ) : null}1283 ) : null}
1264 <MenuSeparator />1284 {pullRequestActionMenuHasGroup(
1285 showsDraftToggle,
1286 showsAutoMerge,
1287 showsMergeMethods,
1288 ) ? (
1289 <MenuSeparator />
1290 ) : null}
1265 </>1291 </>
1266 ) : null}1292 ) : null}
1267 <MenuItem onClick={() => void readLocalApi()?.shell.openExternal(detail.url)}>1293 <MenuItem onClick={() => void readLocalApi()?.shell.openExternal(detail.url)}>
apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts+7
...@@ -19,6 +19,7 @@ import {...@@ -19,6 +19,7 @@ import {
19 isThreadOwnPullRequest,19 isThreadOwnPullRequest,
20 orderPullRequestComments,20 orderPullRequestComments,
21 pullRequestActionNeedsHostRefresh,21 pullRequestActionNeedsHostRefresh,
22 pullRequestActionMenuHasGroup,
22 pullRequestFindingKey,23 pullRequestFindingKey,
23 pullRequestHandoffLabels,24 pullRequestHandoffLabels,
24 readableFailure,25 readableFailure,
...@@ -53,6 +54,12 @@ const TIMELINE_SOURCE: Pick<...@@ -53,6 +54,12 @@ const TIMELINE_SOURCE: Pick<
53 closedAt: null,54 closedAt: null,
54};55};
5556
57describe("pull request action menu", () => {
58 it("keeps the group divider when auto-merge is the only action", () => {
59 expect(pullRequestActionMenuHasGroup(false, true, false)).toBe(true);
60 });
61});
62
56describe("pull request state description", () => {63describe("pull request state description", () => {
57 it("keeps draft and conflicts orthogonal to the terminal states", () => {64 it("keeps draft and conflicts orthogonal to the terminal states", () => {
58 expect(describePullRequestState("open", true)).toBe("Draft");65 expect(describePullRequestState("open", true)).toBe("Draft");
apps/web/src/components/pullRequest/pullRequestDetail.logic.ts+9
...@@ -57,6 +57,15 @@ export function pullRequestHandoffLabels(inThisThread: boolean) {...@@ -57,6 +57,15 @@ export function pullRequestHandoffLabels(inThisThread: boolean) {
57 };57 };
58}58}
5959
60/** Whether the open pull-request action group contains at least one action. */
61export function pullRequestActionMenuHasGroup(
62 showsDraftToggle: boolean,
63 showsAutoMerge: boolean,
64 showsMergeMethods: boolean,
65): boolean {
66 return showsDraftToggle || showsAutoMerge || showsMergeMethods;
67}
68
60/** Plain-language state, shown beside the author. Conflicts are a merge signal, not a state. */69/** Plain-language state, shown beside the author. Conflicts are a merge signal, not a state. */
61export function describePullRequestState(state: PullRequestState, isDraft: boolean): string {70export function describePullRequestState(state: PullRequestState, isDraft: boolean): string {
62 if (state === "merged") return "Merged";71 if (state === "merged") return "Merged";
apps/web/src/components/ui/menu.test.tsx created+23
...@@ -0,0 +1,23 @@
1import { renderToStaticMarkup } from "react-dom/server";
2import { describe, expect, it } from "vite-plus/test";
3
4import { Menu, MenuRadioGroup, MenuRadioItem } from "./menu";
5
6describe("menu radio item geometry", () => {
7 it("keeps radio-item icons on the same text grid as menu items", () => {
8 const html = renderToStaticMarkup(
9 <Menu>
10 <MenuRadioGroup value="merge">
11 <MenuRadioItem value="merge">
12 <span className="flex items-center gap-2">
13 <svg aria-hidden className="size-3.5" />
14 <span>Merge</span>
15 </span>
16 </MenuRadioItem>
17 </MenuRadioGroup>
18 </Menu>,
19 );
20
21 expect(html).toContain("-mx-0.5");
22 });
23});
apps/web/src/components/ui/menu.tsx+1-1
...@@ -166,7 +166,7 @@ function MenuRadioItem({...@@ -166,7 +166,7 @@ function MenuRadioItem({
166 return (166 return (
167 <MenuPrimitive.RadioItem167 <MenuPrimitive.RadioItem
168 className={cn(168 className={cn(
169 "flex min-h-8 in-data-[side=none]:min-w-[calc(var(--anchor-width)+1.25rem)] cursor-pointer items-center rounded-sm px-2 py-1 text-base text-foreground outline-none data-checked:bg-foreground/[0.08] data-disabled:pointer-events-none data-disabled:cursor-not-allowed data-highlighted:bg-accent data-highlighted:text-accent-foreground data-disabled:opacity-64 sm:min-h-7 sm:text-sm [&_svg:not([class*='size-'])]:size-4.5 sm:[&_svg:not([class*='size-'])]:size-4 [&_svg]:pointer-events-none [&_svg]:shrink-0",169 "[&_svg]:-mx-0.5 flex min-h-8 in-data-[side=none]:min-w-[calc(var(--anchor-width)+1.25rem)] cursor-pointer items-center rounded-sm px-2 py-1 text-base text-foreground outline-none data-checked:bg-foreground/[0.08] data-disabled:pointer-events-none data-disabled:cursor-not-allowed data-highlighted:bg-accent data-highlighted:text-accent-foreground data-disabled:opacity-64 sm:min-h-7 sm:text-sm [&_svg:not([class*='size-'])]:size-4.5 sm:[&_svg:not([class*='size-'])]:size-4 [&_svg]:pointer-events-none [&_svg]:shrink-0",
170 className,170 className,
171 )}171 )}
172 data-slot="menu-radio-item"172 data-slot="menu-radio-item"