Skip to content
Merged
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
8 changes: 8 additions & 0 deletions packages/core/src/git/router-schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,18 @@ export const getPrChangedFilesInput = z.object({
export const getPrChangedFilesOutput = z.array(changedFileSchema);

// getPrDiffStatsBatch schemas
//
// Beyond the diff numbers, the batch also carries the PR's live status
// (`state`/`merged`/`draft`) so list cards can render it from the single
// batched GraphQL request instead of firing one REST call per visible PR.
export const prDiffStatsSchema = z.object({
additions: z.number(),
deletions: z.number(),
changedFiles: z.number(),
/** Lowercased GitHub PR state. */
state: z.enum(["open", "closed", "merged"]),
merged: z.boolean(),
draft: z.boolean(),
});
export type PrDiffStats = z.infer<typeof prDiffStatsSchema>;

Expand Down
7 changes: 7 additions & 0 deletions packages/ui/src/features/git-interaction/usePrActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ export function usePrActions(prUrl: string | null) {
trpc.git.getPrDetailsByUrl.queryKey({ prUrl: variables.prUrl }),
getOptimisticPrState(variables.action),
);
// The inbox Pulls list reads PR status from the batched
// `getPrDiffStatsBatch` query (separate cache, 5-min staleTime), so
// patching the detail cache above isn't enough — invalidate the batch
// so the list badge reflects the new state on next view.
void queryClient.invalidateQueries(
trpc.git.getPrDiffStatsBatch.pathFilter(),
);
} else {
toast.error("Failed to update PR", { description: data.message });
}
Expand Down
15 changes: 11 additions & 4 deletions packages/ui/src/features/inbox/components/PullRequestCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { InboxCardTitle } from "@posthog/ui/features/inbox/components/InboxCardT
import { PrDiffStats } from "@posthog/ui/features/inbox/components/PrDiffStats";
import { PriorityMonogram } from "@posthog/ui/features/inbox/components/PriorityMonogram";
import { SuggestedReviewerAvatarStack } from "@posthog/ui/features/inbox/components/SuggestedReviewerAvatarStack";
import { ReportImplementationPrLink } from "@posthog/ui/features/inbox/components/utils/ReportImplementationPrLink";
import { useInboxReportDetailPrefetch } from "@posthog/ui/features/inbox/hooks/useInboxReportDetailPrefetch";
import { useInboxReportArtefacts } from "@posthog/ui/features/inbox/hooks/useInboxReports";
import { Button as UiButton } from "@posthog/ui/primitives/Button";
Expand Down Expand Up @@ -121,10 +122,16 @@ export function PullRequestCard({
>
<Flex align="center" gap="2" className="shrink-0">
{report.implementation_pr_url && (
<PrDiffStats
prUrl={report.implementation_pr_url}
hideWhileLoading
/>
<Flex direction="column" align="end" gap="1" className="shrink-0">
<ReportImplementationPrLink
prUrl={report.implementation_pr_url}
size="sm"
/>
<PrDiffStats
prUrl={report.implementation_pr_url}
hideWhileLoading
/>
</Flex>
)}
<SuggestedReviewerAvatarStack
reportId={report.id}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { GitMergeIcon, GitPullRequestIcon } from "@phosphor-icons/react";
import { parsePrUrl } from "@posthog/core/inbox/reportPresentation";
import { cn } from "@posthog/quill";
import { usePrDetails } from "@posthog/ui/features/git-interaction/usePrDetails";
import { usePrDiffStatsFromBatch } from "@posthog/ui/features/inbox/context/PrDiffStatsBatchContext";
import { Tooltip } from "@radix-ui/themes";

export type ImplementationPrLinkSize = "sm" | "md";
Expand All @@ -13,41 +15,47 @@ interface ReportImplementationPrLinkProps {
onLinkClick?: () => void;
}

function parseGitHubPrReference(prUrl: string): {
reference: string;
prNumber: string;
} {
try {
const parsed = new URL(prUrl);
const match = parsed.pathname.match(
/^\/([^/]+)\/([^/]+)\/pull\/(\d+)(?:$|[/?#])/,
);
if (match) {
return {
reference: `${match[1]}/${match[2]}#${match[3]}`,
prNumber: `#${match[3]}`,
};
}
} catch {
// Fall through to regex fallback for non-URL-safe inputs
}

const prMatch = prUrl.match(/\/pull\/(\d+)(?:$|[/?#])/);
const prNumber = prMatch ? `#${prMatch[1]}` : "PR";
return {
reference: prUrl,
prNumber,
};
}
/** The only states we render a badge for; anything else is treated as unknown. */
const KNOWN_PR_STATES = new Set(["open", "closed", "merged"]);

export function ReportImplementationPrLink({
prUrl,
size = "sm",
onLinkClick,
}: ReportImplementationPrLinkProps) {
const {
meta: { state, merged, draft, isLoading },
} = usePrDetails(prUrl);
// On list surfaces the surrounding `PrDiffStatsBatchContext` already carries
// this PR's status from the single batched request, so read it from there and
// skip the per-PR query. Fall back to `usePrDetails` only on the standalone
// detail view, where no batch provider is mounted.
const batchEntry = usePrDiffStatsFromBatch(prUrl);
const fallback = usePrDetails(batchEntry.hasBatch ? null : prUrl);

const state = batchEntry.hasBatch
? (batchEntry.stats?.state ?? null)
: fallback.meta.state;
Comment thread
andrewm4894 marked this conversation as resolved.
const merged = batchEntry.hasBatch
? (batchEntry.stats?.merged ?? false)
: fallback.meta.merged;
const draft = batchEntry.hasBatch
? (batchEntry.stats?.draft ?? false)
: fallback.meta.draft;
const isLoading = batchEntry.hasBatch
? batchEntry.isLoading && !batchEntry.stats
: fallback.meta.isLoading;

// Only render for a canonical GitHub PR URL. This both keeps the badge in
// sync with the gated "Open in GitHub" action and avoids turning an arbitrary
// (possibly unsafe-scheme) string into a clickable external link.
const prRef = parsePrUrl(prUrl);
if (!prRef) return null;

// `getPrDetailsByUrl` falls back to `{ state: "unknown" }` when the lookup
// fails (gh offline, private repo, unparseable URL), and a batch miss leaves
// state null. Once settled, render nothing for an unresolved state rather
// than a misleading green "open" badge.
if (!isLoading && (state === null || !KNOWN_PR_STATES.has(state))) {
return null;
}

const isSm = size === "sm";

Expand All @@ -63,15 +71,18 @@ export function ReportImplementationPrLink({
? "bg-gray-4 text-gray-11 hover:bg-gray-5"
: "bg-green-4 text-green-11 hover:bg-green-5";

const { reference: prReference, prNumber } = parseGitHubPrReference(prUrl);
const prReference = `${prRef.repoSlug}#${prRef.number}`;
const prNumber = `#${prRef.number}`;

const tooltip = merged
? `Merged – ${prReference}`
: state === "closed"
? `Closed – ${prReference}`
: draft
? `Draft – ${prReference}`
: `Open – ${prReference}`;
const tooltip = isLoading
? prReference
: merged
? `Merged – ${prReference}`
: state === "closed"
? `Closed – ${prReference}`
: draft
? `Draft – ${prReference}`
: `Open – ${prReference}`;

const iconSize = isSm ? 10 : 12;

Expand Down
8 changes: 8 additions & 0 deletions packages/workspace-server/src/services/git/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,18 @@ export type PrDetailsByUrlOutput = z.infer<typeof getPrDetailsByUrlOutput>;

export const getPrChangedFilesInput = z.object({ prUrl: z.string() });

// Also carries the PR's live status (`state`/`merged`/`draft`) so list cards
// can render it from the single batched GraphQL request instead of firing one
// REST call per visible PR. Mirrors `prDiffStatsSchema` in
// `@posthog/core/git/router-schemas`.
export const prDiffStatsSchema = z.object({
additions: z.number(),
deletions: z.number(),
changedFiles: z.number(),
/** Lowercased GitHub PR state. */
state: z.enum(["open", "closed", "merged"]),
merged: z.boolean(),
draft: z.boolean(),
});
export type PrDiffStats = z.infer<typeof prDiffStatsSchema>;

Expand Down
27 changes: 26 additions & 1 deletion packages/workspace-server/src/services/git/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,24 @@ function toUnifiedDiffPatch(
return `diff --git a/${oldPath} b/${filename}\n--- ${fromPath}\n+++ ${toPath}\n${rawPatch}`;
}

/**
* Narrow GitHub GraphQL's `PullRequestState` (OPEN | CLOSED | MERGED) to the
* lowercased literal the batch schema expects. Anything unexpected falls back
* to "open" so one odd value can never fail validation for the whole batch.
*/
function normalizeGraphqlPrState(
graphqlState: string,
): "open" | "closed" | "merged" {
switch (graphqlState) {
case "MERGED":
return "merged";
case "CLOSED":
return "closed";
default:
return "open";
}
}

export function mapPrState(
state: string | null,
merged: boolean,
Expand Down Expand Up @@ -1054,7 +1072,7 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {
): Promise<Record<string, PrDiffStats>> {
const aliasFragments = chunk
.map(([, { parsed }], index) => {
return `pr${index}: repository(owner: "${escapeGraphqlString(parsed.owner)}", name: "${escapeGraphqlString(parsed.repo)}") { pullRequest(number: ${parsed.number}) { additions deletions changedFiles } }`;
return `pr${index}: repository(owner: "${escapeGraphqlString(parsed.owner)}", name: "${escapeGraphqlString(parsed.repo)}") { pullRequest(number: ${parsed.number}) { additions deletions changedFiles state isDraft } }`;
})
.join("\n");
const query = `query InboxPrDiffStatsBatch {\n${aliasFragments}\n}`;
Expand All @@ -1075,6 +1093,8 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {
additions: number;
deletions: number;
changedFiles: number;
state: string;
isDraft: boolean;
} | null;
} | null
>;
Expand All @@ -1085,10 +1105,15 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {
const [, { urls }] = chunk[i];
const node = parsed.data?.[`pr${i}`]?.pullRequest;
if (!node) continue;
// GraphQL `PullRequestState` is OPEN | CLOSED | MERGED; normalise to the
// lowercase state + merged boolean shape the badge expects.
const stats: PrDiffStats = {
additions: node.additions,
deletions: node.deletions,
changedFiles: node.changedFiles,
state: normalizeGraphqlPrState(node.state),
merged: node.state === "MERGED",
draft: node.isDraft,
};
for (const url of urls) {
out[url] = stats;
Expand Down
Loading