Skip to content

Commit df31d00

Browse files
AviPeltzAviPeltz
committed
feat(web): view any GitHub PR's description and diff from its link
New /pr page: paste a github.com pull request URL and it renders that PR's own description and diff, fetched live from GitHub (org's GitHub App installation first, falling back to unauthenticated REST for public repos) — independent of whether the PR is tracked in Superset's DB. Reuses the existing pixel-matched review-report HTML renderer, extended with a small markdown-to-HTML converter for the description.
1 parent 62d5595 commit df31d00

11 files changed

Lines changed: 634 additions & 8 deletions

File tree

‎apps/web/src/app/pr/components/PrViewer/PrViewer.tsx‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
"use client";
2+
3+
import { parseGithubPullRequestUrl } from "@superset/shared/github-pr-url";
4+
import { renderReviewReportHtml } from "@superset/shared/review-report";
5+
import { Button } from "@superset/ui/button";
6+
import { Input } from "@superset/ui/input";
7+
import { useQuery } from "@tanstack/react-query";
8+
import { TRPCClientError } from "@trpc/client";
9+
import Link from "next/link";
10+
import { useRouter, useSearchParams } from "next/navigation";
11+
import { type FormEvent, useMemo, useState } from "react";
12+
import { useTRPC } from "@/trpc/react";
13+
14+
function errorInfo(error: unknown): { message: string; unauthorized: boolean } {
15+
if (error instanceof TRPCClientError) {
16+
return {
17+
message: error.message,
18+
unauthorized: error.data?.code === "UNAUTHORIZED",
19+
};
20+
}
21+
return {
22+
message: "Something went wrong loading this pull request.",
23+
unauthorized: false,
24+
};
25+
}
26+
27+
export function PrViewer() {
28+
const router = useRouter();
29+
const searchParams = useSearchParams();
30+
const trpc = useTRPC();
31+
32+
const urlParam = searchParams.get("url") ?? "";
33+
const [inputValue, setInputValue] = useState(urlParam);
34+
const isValidUrl = parseGithubPullRequestUrl(urlParam) !== null;
35+
36+
const query = useQuery({
37+
...trpc.githubPr.fetchByUrl.queryOptions({ prUrl: urlParam }),
38+
enabled: isValidUrl,
39+
retry: false,
40+
});
41+
42+
const html = useMemo(() => {
43+
const pr = query.data;
44+
if (!pr) return null;
45+
return renderReviewReportHtml({
46+
title: pr.title,
47+
repo: `${pr.owner}/${pr.repo}`,
48+
prNumber: pr.number,
49+
prUrl: pr.htmlUrl,
50+
branch: pr.headBranch,
51+
generatedAt: pr.updatedAt,
52+
description: pr.description ?? undefined,
53+
diff: pr.diff,
54+
});
55+
}, [query.data]);
56+
57+
function navigateTo(value: string) {
58+
const trimmed = value.trim();
59+
const params = new URLSearchParams(searchParams);
60+
if (trimmed) params.set("url", trimmed);
61+
else params.delete("url");
62+
router.push(`/pr${params.size > 0 ? `?${params.toString()}` : ""}`);
63+
}
64+
65+
function handleSubmit(event: FormEvent) {
66+
event.preventDefault();
67+
navigateTo(inputValue);
68+
}
69+
70+
const searchForm = (
71+
<form onSubmit={handleSubmit} className="flex w-full gap-2">
72+
<Input
73+
value={inputValue}
74+
onChange={(event) => setInputValue(event.target.value)}
75+
placeholder="https://github.com/owner/repo/pull/123"
76+
aria-label="GitHub pull request URL"
77+
/>
78+
<Button type="submit">View</Button>
79+
</form>
80+
);
81+
82+
if (!urlParam || !isValidUrl) {
83+
return (
84+
<div className="flex h-dvh flex-col items-center justify-center gap-4 px-4">
85+
<div className="w-full max-w-md space-y-3">
86+
<h1 className="text-center font-semibold text-xl">
87+
View a pull request
88+
</h1>
89+
<p className="text-center text-muted-foreground text-sm">
90+
Paste a GitHub pull request link to view its description and diff.
91+
</p>
92+
{searchForm}
93+
{urlParam && !isValidUrl ? (
94+
<p className="text-center text-destructive text-sm">
95+
That doesn't look like a github.com pull request link.
96+
</p>
97+
) : null}
98+
</div>
99+
</div>
100+
);
101+
}
102+
103+
if (query.isPending) {
104+
return (
105+
<div className="flex h-dvh items-center justify-center text-muted-foreground text-sm">
106+
Loading pull request…
107+
</div>
108+
);
109+
}
110+
111+
if (query.isError) {
112+
const { message, unauthorized } = errorInfo(query.error);
113+
return (
114+
<div className="flex h-dvh flex-col items-center justify-center gap-3 px-4 text-center">
115+
<p className="max-w-md text-sm">{message}</p>
116+
{unauthorized ? (
117+
<Button asChild>
118+
<Link
119+
href={`/sign-in?redirect=${encodeURIComponent(`/pr?url=${urlParam}`)}`}
120+
>
121+
Sign in
122+
</Link>
123+
</Button>
124+
) : (
125+
<Button variant="outline" onClick={() => navigateTo("")}>
126+
Try another link
127+
</Button>
128+
)}
129+
</div>
130+
);
131+
}
132+
133+
return (
134+
<div className="flex h-dvh flex-col">
135+
<div className="flex shrink-0 items-center gap-2 border-b px-3 py-2">
136+
{searchForm}
137+
</div>
138+
<main className="min-h-0 flex-1">
139+
{html ? (
140+
<iframe
141+
srcDoc={html}
142+
title={query.data?.title ?? "Pull request"}
143+
className="h-full w-full border-0"
144+
// No allow-scripts/allow-same-origin: the diff and description
145+
// are someone else's PR content, not ours. Tabs and collapsible
146+
// sections are pure CSS/native <details>, so they still work.
147+
// allow-popups(-to-escape-sandbox) only lets the target="_blank"
148+
// links open real, unsandboxed new tabs.
149+
sandbox="allow-popups allow-popups-to-escape-sandbox"
150+
/>
151+
) : null}
152+
</main>
153+
</div>
154+
);
155+
}

‎apps/web/src/app/pr/components/PrViewer/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export { PrViewer } from "./PrViewer";

‎apps/web/src/app/pr/page.tsx‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import type { Metadata } from "next";
2+
import { Suspense } from "react";
3+
import { PrViewer } from "./components/PrViewer";
4+
5+
export const metadata: Metadata = {
6+
title: "View a pull request",
7+
};
8+
9+
export default function PrPage() {
10+
return (
11+
<Suspense fallback={null}>
12+
<PrViewer />
13+
</Suspense>
14+
);
15+
}

‎bun.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/shared/src/review-report.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,60 @@ describe("renderReviewReportHtml", () => {
303303
expect(lineCount).toBe(3);
304304
});
305305

306+
it("renders a plain PR's markdown description instead of the findings empty state when no findings are given", () => {
307+
const html = renderReviewReportHtml({
308+
title: "Add caching layer",
309+
generatedAt: "2026-01-01T00:00:00.000Z",
310+
description:
311+
"## Summary\n\nAdds a **cache** with `LRU` eviction.\n\n- fast\n- simple",
312+
});
313+
expect(html).not.toContain("No findings");
314+
expect(html).toContain('<div class="markdown">');
315+
expect(html).toContain("<h2>Summary</h2>");
316+
expect(html).toContain(
317+
"Adds a <strong>cache</strong> with <code>LRU</code> eviction.",
318+
);
319+
expect(html).toContain("<li>fast</li>");
320+
expect(html).toContain("<li>simple</li>");
321+
});
322+
323+
it("omits the findings pill for a plain PR description view", () => {
324+
const html = renderReviewReportHtml({
325+
title: "Add caching layer",
326+
generatedAt: "2026-01-01T00:00:00.000Z",
327+
description: "Plain body.",
328+
});
329+
expect(html).not.toContain('class="pill');
330+
});
331+
332+
it("prefers findings over a description when both are given", () => {
333+
const html = renderReviewReportHtml({
334+
title: "Fix bug",
335+
generatedAt: "2026-01-01T00:00:00.000Z",
336+
description: "Should not render.",
337+
findings: [
338+
{
339+
file: "a.ts",
340+
summary: "confirmed issue",
341+
failureScenario: "n/a",
342+
verdict: "CONFIRMED",
343+
},
344+
],
345+
});
346+
expect(html).toContain("confirmed issue");
347+
expect(html).not.toContain('<div class="markdown">');
348+
});
349+
350+
it("escapes HTML in a markdown description and in link/code spans", () => {
351+
const html = renderReviewReportHtml({
352+
title: "Fix bug",
353+
generatedAt: "2026-01-01T00:00:00.000Z",
354+
description: "<script>alert(1)</script>\n\n[click](javascript:alert(1))",
355+
});
356+
expect(html).not.toContain("<script>alert(1)</script>");
357+
expect(html).toContain("&lt;script&gt;alert(1)&lt;/script&gt;");
358+
});
359+
306360
it("resolves the full path when it contains a literal ' b/' substring", () => {
307361
const diff = [
308362
"diff --git a/a.ts b/a b/c.ts",

0 commit comments

Comments
 (0)