authorgravatar for 62337003+Bil0000@users.noreply.github.comBilal Bakr <62337003+Bil0000@users.noreply.github.com> 2026-09-14 20:46:28+03:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-14 14:46:28-03:00
log47ace94962a714a561d7cfbdbaa4c721ef6b0598
tree20a976f04f86931bf5b462f62a06e6bc4efc8ff4
parent494cfac2494095c3f9b24cc16fb73721f84894a4
signature Signed by PGP key B5690EEEBB952194

fix(web): use consistent PR section toggles (#11763)

2 files changed, 193 insertions(+), 72 deletions(-)

apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx created+141
......@@ -0,0 +1,141 @@
1import { EnvironmentId, ProjectId, type PullRequestDetailView } from "@t3tools/contracts";
2import { act } from "react";
3import { create, type ReactTestRenderer } from "react-test-renderer";
4import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test";
5
6vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() }));
7vi.mock("~/state/pullRequests", () => ({ pullRequestEnvironment: {} }));
8vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => vi.fn() }));
9vi.mock("./PullRequestMarkdown", () => ({
10 PullRequestMarkdown: ({ text }: { text: string }) => <p>{text}</p>,
11}));
12
13import { PullRequestSummaryTab } from "./PullRequestSummaryTab";
14
15const detail: PullRequestDetailView = {
16 provider: "github",
17 projectId: ProjectId.make("project"),
18 projectTitle: "Project",
19 workspaceRoot: "/workspace",
20 repository: "owner/repo",
21 number: 1,
22 title: "Test pull request",
23 body: "Original description",
24 url: "https://github.com/owner/repo/pull/1",
25 author: { login: "author", name: null, avatarUrl: null },
26 viewer: "author",
27 state: "open",
28 isDraft: false,
29 mergeability: "mergeable",
30 additions: 1,
31 deletions: 0,
32 changedFiles: 1,
33 headBranch: "feature",
34 baseBranch: "main",
35 createdAt: "2026-09-01T00:00:00Z",
36 updatedAt: "2026-09-01T00:00:00Z",
37 mergedAt: null,
38 closedAt: null,
39 reviewers: [],
40 labels: [],
41 checks: [{ name: "Unit tests", status: "success", description: null, url: null }],
42 comments: [],
43 commentCount: 0,
44 commentsTruncated: false,
45 reviewThreads: [],
46 commits: [],
47 mergeCapabilities: { merge: false, squash: false, rebase: false },
48 capabilities: {
49 diff: true,
50 comment: true,
51 search: true,
52 actions: [],
53 mergeMethods: [],
54 review: { inlineComment: false, reply: false, resolve: false, verdicts: [] },
55 reviewers: { request: false, listCandidates: false },
56 edit: { changeRequest: true, comment: true },
57 },
58 viewerPermissions: {
59 actions: [],
60 comment: true,
61 resolve: false,
62 verdicts: [],
63 requestReviewers: false,
64 },
65};
66
67let renderer: ReactTestRenderer;
68beforeEach(() => vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true));
69afterEach(() => {
70 act(() => renderer?.unmount());
71 vi.unstubAllGlobals();
72});
73
74function render(value = detail) {
75 return (
76 <PullRequestSummaryTab
77 environmentId={EnvironmentId.make("environment")}
78 threadRef={null}
79 reference={value}
80 detail={value}
81 activityPending={false}
82 activityError={null}
83 onRefresh={() => {}}
84 />
85 );
86}
87
88function heading(title: string) {
89 const section = renderer.root
90 .findAllByType("section")
91 .find((section) => section.props["aria-label"] === title)!;
92 return section
93 .findAllByType("button")
94 .find((button) => button.findAllByType("span").some((span) => span.children.includes(title)))!;
95}
96
97function click(title: string) {
98 act(() =>
99 heading(title).props.onClick({ nativeEvent: {}, preventDefault() {}, stopPropagation() {} }),
100 );
101}
102
103it("toggles checks from their heading and resets sections for another pull request", () => {
104 act(() => {
105 renderer = create(render());
106 });
107 expect(
108 renderer.root.findAllByType("span").some((span) => span.children.includes("Unit tests")),
109 ).toBe(false);
110 click("Checks");
111 expect(
112 renderer.root.findAllByType("span").some((span) => span.children.includes("Unit tests")),
113 ).toBe(true);
114 click("Checks");
115 expect(heading("Checks").props["aria-expanded"]).toBe(false);
116 click("Checks");
117 click("Description");
118 act(() =>
119 renderer.update(render({ ...detail, url: "https://github.com/owner/repo/pull/2", number: 2 })),
120 );
121 expect(heading("Checks").props["aria-expanded"]).toBe(false);
122 expect(heading("Description").props["aria-expanded"]).toBe(true);
123});
124
125it("keeps an unsaved description when collapsed and reopened", () => {
126 act(() => {
127 renderer = create(render());
128 });
129 act(() => renderer.root.findByProps({ "aria-label": "Edit description" }).props.onClick());
130 act(() =>
131 renderer.root.findByType("textarea").props.onChange({
132 target: { value: "Unsaved description" },
133 currentTarget: { value: "Unsaved description" },
134 nativeEvent: {},
135 }),
136 );
137 click("Description");
138 expect(heading("Description").props["aria-expanded"]).toBe(false);
139 click("Description");
140 expect(renderer.root.findByType("textarea").props.value).toBe("Unsaved description");
141});
apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx+52-72
......@@ -15,7 +15,7 @@ import {
1515 TagIcon,
1616 UsersIcon,
1717} from "lucide-react";
18import { useId, useRef, useState, type ReactNode } from "react";
18import { useRef, useState, type ReactNode } from "react";
1919
2020import { useAtomCommand } from "~/state/use-atom-command";
2121import { pullRequestEnvironment } from "~/state/pullRequests";
......@@ -237,11 +237,13 @@ function MetaRow({
237237function Section({
238238 title,
239239 defaultOpen = true,
240 keepMounted = false,
240241 actions,
241242 children,
242243}: {
243244 title: string;
244245 defaultOpen?: boolean;
246 keepMounted?: boolean;
245247 /** Heading controls stay separate from the collapse trigger so they remain independently usable. */
246248 actions?: ReactNode;
247249 children: ReactNode;
......@@ -271,6 +273,7 @@ function Section({
271273 <Collapsible
272274 open={open}
273275 onOpenChange={setOpenWithScrollAnchor}
276 render={<section aria-label={title} />}
274277 data-pull-request-summary-section
275278 >
276279 {/* The heading rides the top of the scroll box the way a diff's file header does, so a
......@@ -292,7 +295,7 @@ function Section({
292295 </CollapsibleTrigger>
293296 {actions}
294297 </div>
295 <CollapsiblePanel>
298 <CollapsiblePanel keepMounted={keepMounted}>
296299 <div className="px-4 pb-4">{children}</div>
297300 </CollapsiblePanel>
298301 </Collapsible>
......@@ -334,9 +337,6 @@ export function PullRequestSummaryTab({
334337 // Keyed by the pull request, so opening another one starts at the end of its conversation
335338 // rather than wherever the last one had been read back to.
336339 const [shown, setShown] = useState({ url: detail.url, count: COMMENT_PAGE });
337 const checksId = useId();
338 const [expandedChecksUrl, setExpandedChecksUrl] = useState<string | null>(null);
339 const showChecks = expandedChecksUrl === detail.url;
340340 const shownComments = shown.url === detail.url ? shown.count : COMMENT_PAGE;
341341 // Windowed by recency regardless of display order: expanding always reaches further back in
342342 // time, whether the newest comment currently reads first or last.
......@@ -472,7 +472,7 @@ export function PullRequestSummaryTab({
472472
473473 return (
474474 <div className="h-full overflow-y-auto" data-pull-request-summary-scroll>
475 <section className="px-4 py-2.5">
475 <section className="px-4 pt-2.5 pb-1">
476476 <div className="space-y-2">
477477 <MetaRow icon={<UsersIcon className="size-3.5" />} label="Reviewers">
478478 <span className="flex min-w-0 flex-wrap items-center gap-1.5">
......@@ -601,7 +601,7 @@ export function PullRequestSummaryTab({
601601 </div>
602602 </section>
603603
604 <section aria-label="Description" className="px-4 pt-2 pb-1">
604 <Section key={`description:${detail.url}`} title="Description" keepMounted>
605605 <div className="group">
606606 {bodyScope === detail.url ? (
607607 <PullRequestMarkdownEditor
......@@ -640,78 +640,58 @@ export function PullRequestSummaryTab({
640640 </div>
641641 )}
642642 </div>
643 </section>
643 </Section>
644644
645 <section aria-label="Checks" className="px-4 py-3">
645 <Section key={`checks:${detail.url}`} title="Checks" defaultOpen={false}>
646646 {detail.checks.length === 0 ? (
647647 <p className="text-xs text-muted-foreground">No checks reported.</p>
648648 ) : (
649 <div>
650 <div className="flex items-center gap-1 text-xs">
651 <span className="font-medium text-muted-foreground">Checks</span>
652 <Button
653 size="icon-xs"
654 variant="ghost-muted"
655 aria-label={showChecks ? "Hide checks" : "Show checks"}
656 aria-expanded={showChecks}
657 aria-controls={checksId}
658 onClick={() => setExpandedChecksUrl(showChecks ? null : detail.url)}
649 detail.checks.map((check, index) => {
650 const finding = { kind: "check", check } as const;
651 const failing = check.status === "failure" || check.status === "cancelled";
652 return (
653 <div
654 // Position too: the host decides how many runs share a name, and a repeated
655 // key would be a rendering fault on top of whatever the list already says.
656 key={`${index}:${check.name}:${check.url ?? ""}`}
657 className="group flex items-center gap-2 rounded-md pr-1 hover:bg-accent/60"
659658 >
660 <ChevronRightIcon
661 aria-hidden
662 className={cn("size-3.5 text-muted-foreground/60", showChecks && "rotate-90")}
663 />
664 </Button>
665 </div>
666 <div id={checksId} className={showChecks ? "mt-2" : "hidden"}>
667 {(showChecks ? detail.checks : []).map((check, index) => {
668 const finding = { kind: "check", check } as const;
669 const failing = check.status === "failure" || check.status === "cancelled";
670 return (
671 <div
672 // Position too: the host decides how many runs share a name, and a repeated
673 // key would be a rendering fault on top of whatever the list already says.
674 key={`${index}:${check.name}:${check.url ?? ""}`}
675 className="group flex items-center gap-2 rounded-md pr-1 hover:bg-accent/60"
676 >
677 <button
678 type="button"
679 disabled={!check.url}
680 onClick={() => check.url && openCheck(check.url)}
681 className={cn(
682 "flex min-w-0 flex-1 items-start gap-2 rounded-md px-2 py-2 text-left text-xs leading-5 [&>svg]:mt-0.5",
683 check.url ? "cursor-pointer" : "cursor-default",
684 )}
685 >
686 <PullRequestCheckStatusIcon status={check.status} />
687 <span className="min-w-0 flex-1 wrap-anywhere">{check.name}</span>
688 <span className="shrink-0 text-muted-foreground">
689 {pullRequestCheckStatusLabel(check)}
690 </span>
691 </button>
692 {/* Only where there is something to fix. A passing check has no failure to
659 <button
660 type="button"
661 disabled={!check.url}
662 onClick={() => check.url && openCheck(check.url)}
663 className={cn(
664 "flex min-w-0 flex-1 items-start gap-2 rounded-md px-2 py-2 text-left text-xs leading-5 [&>svg]:mt-0.5",
665 check.url ? "cursor-pointer" : "cursor-default",
666 )}
667 >
668 <PullRequestCheckStatusIcon status={check.status} />
669 <span className="min-w-0 flex-1 wrap-anywhere">{check.name}</span>
670 <span className="shrink-0 text-muted-foreground">
671 {pullRequestCheckStatusLabel(check)}
672 </span>
673 </button>
674 {/* Only where there is something to fix. A passing check has no failure to
693675 reproduce, and the button would be an invitation to waste a thread. */}
694 {onFixFinding && failing ? (
695 <Button
696 size="xs"
697 variant="ghost"
698 className="shrink-0"
699 disabled={pendingFinding !== null && pendingFinding !== undefined}
700 onClick={() => onFixFinding(finding)}
701 >
702 <HammerIcon className="size-3" />
703 {pendingFinding === pullRequestFindingKey(finding)
704 ? "Preparing..."
705 : fixCheckLabel}
706 </Button>
707 ) : null}
708 </div>
709 );
710 })}
711 </div>
712 </div>
676 {onFixFinding && failing ? (
677 <Button
678 size="xs"
679 variant="ghost"
680 className="shrink-0"
681 disabled={pendingFinding !== null && pendingFinding !== undefined}
682 onClick={() => onFixFinding(finding)}
683 >
684 <HammerIcon className="size-3" />
685 {pendingFinding === pullRequestFindingKey(finding)
686 ? "Preparing..."
687 : fixCheckLabel}
688 </Button>
689 ) : null}
690 </div>
691 );
692 })
713693 )}
714 </section>
694 </Section>
715695
716696 <Section
717697 title="Comments"