diff --git a/src/renderer/hooks/usePendingPrRefresh.test.tsx b/src/renderer/hooks/usePendingPrRefresh.test.tsx index 05d4480c..06073240 100644 --- a/src/renderer/hooks/usePendingPrRefresh.test.tsx +++ b/src/renderer/hooks/usePendingPrRefresh.test.tsx @@ -142,4 +142,30 @@ describe("usePendingPrRefresh", () => { expect(ghGetPrForBranchMock).not.toHaveBeenCalled(); expect(ghGetPrDetailsMock).not.toHaveBeenCalled(); }); + + it("refetches when cached PR details still have pending checks", async () => { + useGitStore.getState().setPrData("pr-key", { ...basePr, checksStatus: "SUCCESS" }); + useGitStore.getState().setPrDetails("p1#42", baseDetails); + ghGetPrDetailsMock.mockResolvedValueOnce({ + details: { + ...baseDetails, + checks: [{ name: "CI", state: "COMPLETED", conclusion: "SUCCESS" }], + }, + }); + + renderHook(() => + usePendingPrRefresh({ + prKey: "pr-key", + projectLocation: location, + branch: undefined, + cacheKey: "p1#42", + }), + ); + + await act(async () => {}); + + expect(ghGetPrForBranchMock).not.toHaveBeenCalled(); + expect(ghGetPrDetailsMock).toHaveBeenCalledWith({ projectLocation: location, prNumber: 42 }); + expect(useGitStore.getState().prDetails["p1#42"]?.checks[0]?.conclusion).toBe("SUCCESS"); + }); }); diff --git a/src/renderer/hooks/usePendingPrRefresh.ts b/src/renderer/hooks/usePendingPrRefresh.ts index 7713a007..95483d12 100644 --- a/src/renderer/hooks/usePendingPrRefresh.ts +++ b/src/renderer/hooks/usePendingPrRefresh.ts @@ -2,8 +2,9 @@ import { useEffect } from "react"; import type { ProjectLocation } from "@/shared/contracts"; import { readBridge } from "@/renderer/bridge"; import { useGitStore } from "@/renderer/state/gitStore"; -import { usePrChecksStatus, usePrNumber, usePrState } from "@/renderer/state/gitSelectors"; +import { usePrNumber, usePrState } from "@/renderer/state/gitSelectors"; import { getPrStatusTone } from "@/renderer/utils/prStatus"; +import { usePrCombinedChecksStatus } from "./usePrCombinedChecksStatus"; export const PR_PENDING_REFRESH_INTERVAL_MS = 30_000; @@ -16,15 +17,18 @@ export function usePendingPrRefresh(params: { const { prKey, projectLocation, branch, cacheKey } = params; const state = usePrState(prKey); const number = usePrNumber(prKey); - const checksStatus = usePrChecksStatus(prKey); + const details = useGitStore((s) => (cacheKey ? s.prDetails[cacheKey] : undefined)); + const effectiveChecksStatus = usePrCombinedChecksStatus(prKey, cacheKey); + const prNumber = number ?? details?.number; useEffect(() => { - if (!prKey || getPrStatusTone(state, checksStatus) !== "warning") return; + if (!prKey || getPrStatusTone(state, effectiveChecksStatus) !== "warning") return; const targetPrKey = prKey; const targetBranch = branch; const detailsCacheKey = cacheKey; - const detailsPrNumber = number; + const detailsPrNumber = prNumber; + if (!targetBranch && (!detailsCacheKey || !detailsPrNumber)) return; let cancelled = false; let inFlight = false; @@ -44,11 +48,11 @@ export function usePendingPrRefresh(params: { .ghGetPrDetails({ projectLocation, prNumber: detailsPrNumber }) .catch(() => undefined) : Promise.resolve(undefined); - const [pr, details] = await Promise.all([prPromise, detailsPromise]); + const [pr, refreshedDetails] = await Promise.all([prPromise, detailsPromise]); if (cancelled) return; if (pr !== undefined) useGitStore.getState().setPrData(targetPrKey, pr); - if (detailsCacheKey && details) { - useGitStore.getState().setPrDetails(detailsCacheKey, details.details); + if (detailsCacheKey && refreshedDetails) { + useGitStore.getState().setPrDetails(detailsCacheKey, refreshedDetails.details); } } finally { inFlight = false; @@ -64,5 +68,5 @@ export function usePendingPrRefresh(params: { cancelled = true; window.clearInterval(intervalId); }; - }, [branch, cacheKey, checksStatus, number, prKey, projectLocation, state]); + }, [branch, cacheKey, effectiveChecksStatus, prKey, prNumber, projectLocation, state]); } diff --git a/src/renderer/hooks/usePrCombinedChecksStatus.ts b/src/renderer/hooks/usePrCombinedChecksStatus.ts new file mode 100644 index 00000000..25a46e79 --- /dev/null +++ b/src/renderer/hooks/usePrCombinedChecksStatus.ts @@ -0,0 +1,10 @@ +import { useGitStore } from "@/renderer/state/gitStore"; +import { usePrChecksStatus } from "@/renderer/state/gitSelectors"; +import { aggregatePrChecksStatus, combineChecksStatus } from "@/renderer/utils/prStatus"; + +export function usePrCombinedChecksStatus(prKey: string | undefined, cacheKey: string | undefined) { + const prChecksStatus = usePrChecksStatus(prKey); + const detailsChecks = useGitStore((s) => (cacheKey ? s.prDetails[cacheKey]?.checks : undefined)); + const detailsChecksStatus = aggregatePrChecksStatus(detailsChecks); + return combineChecksStatus(detailsChecksStatus, prChecksStatus); +} diff --git a/src/renderer/utils/prStatus.ts b/src/renderer/utils/prStatus.ts index 3786eea5..8738d5ce 100644 --- a/src/renderer/utils/prStatus.ts +++ b/src/renderer/utils/prStatus.ts @@ -1,6 +1,58 @@ -import type { PrState } from "@/shared/contracts"; +import { PR_CHECK_FAILURE_CONCLUSIONS, type PrCheck, type PrState } from "@/shared/contracts"; export type PrStatusTone = "merged" | "draft" | "danger" | "warning" | "success"; +export type PrChecksStatus = "FAILURE" | "PENDING" | "SUCCESS"; +export type PrChecksTone = "danger" | "warning" | "success"; + +export function aggregatePrChecksStatus( + checks: readonly PrCheck[] | undefined, +): PrChecksStatus | undefined { + if (!checks || checks.length === 0) return undefined; + let hasPending = false; + for (const check of checks) { + const conclusion = check.conclusion.toUpperCase(); + const state = check.state.toUpperCase(); + if (PR_CHECK_FAILURE_CONCLUSIONS.has(conclusion) || state === "ERROR" || state === "FAILURE") { + return "FAILURE"; + } + if (state && state !== "COMPLETED" && state !== "SUCCESS") { + hasPending = true; + } + } + return hasPending ? "PENDING" : "SUCCESS"; +} + +const CHECKS_STATUS_TONE: Record = { + FAILURE: "danger", + PENDING: "warning", + SUCCESS: "success", +}; + +function normalizeChecksStatus(checksStatus: string | undefined): PrChecksStatus | undefined { + const status = checksStatus?.toUpperCase(); + if (status === "FAILURE" || status === "ERROR") return "FAILURE"; + if (status === "PENDING") return "PENDING"; + if (status === "SUCCESS") return "SUCCESS"; + return undefined; +} + +export function getChecksStatusTone( + checksStatus: PrChecksStatus | undefined, +): PrChecksTone | undefined { + return checksStatus ? CHECKS_STATUS_TONE[checksStatus] : undefined; +} + +export function combineChecksStatus( + detailsStatus: string | undefined, + prStatus: string | undefined, +): PrChecksStatus | undefined { + const normalizedDetails = normalizeChecksStatus(detailsStatus); + const normalizedPr = normalizeChecksStatus(prStatus); + if (normalizedDetails === "FAILURE" || normalizedPr === "FAILURE") return "FAILURE"; + if (normalizedDetails === "PENDING" || normalizedPr === "PENDING") return "PENDING"; + if (normalizedDetails === "SUCCESS" || normalizedPr === "SUCCESS") return "SUCCESS"; + return undefined; +} export function getPrStatusTone( state: PrState | undefined, @@ -8,8 +60,8 @@ export function getPrStatusTone( ): PrStatusTone { if (state === "merged") return "merged"; if (state === "draft") return "draft"; - if (checksStatus === "FAILURE" || checksStatus === "ERROR") return "danger"; - if (checksStatus === "PENDING") return "warning"; + const checksTone = getChecksStatusTone(normalizeChecksStatus(checksStatus)); + if (checksTone) return checksTone; return "success"; } diff --git a/src/renderer/views/PrReviewOverlay/PrReviewOverlay.tsx b/src/renderer/views/PrReviewOverlay/PrReviewOverlay.tsx index bb06efbf..6b70c1fa 100644 --- a/src/renderer/views/PrReviewOverlay/PrReviewOverlay.tsx +++ b/src/renderer/views/PrReviewOverlay/PrReviewOverlay.tsx @@ -5,6 +5,8 @@ import type { Project, ProjectLocation } from "@/shared/contracts"; import { friendlyError } from "@/shared/messages"; import { readBridge } from "@/renderer/bridge"; import { useGitStore } from "@/renderer/state/gitStore"; +import { usePendingPrRefresh } from "@/renderer/hooks/usePendingPrRefresh"; +import { usePrCombinedChecksStatus } from "@/renderer/hooks/usePrCombinedChecksStatus"; import { PageLayout } from "@/renderer/components/layout/PageLayout"; import { usePrTitle, usePrUrl, usePrViewerDidAuthor } from "@/renderer/state/gitSelectors"; import { PrReviewSidebar } from "./parts/PrReviewSidebar"; @@ -36,6 +38,7 @@ export function PrReviewOverlay(props: { const files = useGitStore((s) => s.prFiles[cacheKey]); const rawDiff = useGitStore((s) => s.prDiffs[cacheKey]); const details = useGitStore((s) => s.prDetails[cacheKey]); + const combinedChecksStatus = usePrCombinedChecksStatus(prKey, cacheKey); const prTitleFromData = usePrTitle(prKey); const prTitle = prTitleFromData || details?.title || ""; const prUrl = usePrUrl(prKey); @@ -46,6 +49,13 @@ export function PrReviewOverlay(props: { const [loading, setLoading] = useState(false); const [activeTab, setActiveTab] = useState("conversation"); + usePendingPrRefresh({ + prKey, + projectLocation: effectiveLocation, + branch: details?.headBranch, + ...(details ? { cacheKey } : {}), + }); + // Track content width to decide if the meta row fits inline in the content // header or should overflow into a row above the tabs. Above ~880px the // chips, branches, and stats all fit comfortably; below, they're pushed down. @@ -230,6 +240,7 @@ export function PrReviewOverlay(props: { active={activeTab} onChange={setActiveTab} counts={counts} + checksStatus={combinedChecksStatus} pillInHeaderBreakpoint="never" conversationPanel={ void; counts: PrTabCounts; + checksStatus?: PrChecksStatus | undefined; conversationPanel: ReactNode; commitsPanel: ReactNode; checksPanel: ReactNode; @@ -18,6 +20,7 @@ export function PrTabs(props: { active, onChange, counts, + checksStatus, conversationPanel, commitsPanel, checksPanel, @@ -50,7 +53,12 @@ export function PrTabs(props: { return (
- +
{panel} diff --git a/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.test.tsx b/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.test.tsx new file mode 100644 index 00000000..5b61a8b0 --- /dev/null +++ b/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.test.tsx @@ -0,0 +1,37 @@ +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; +import type { PrChecksStatus } from "@/renderer/utils/prStatus"; +import { PrTabsPill, type PrTabCounts } from "./PrTabsPill"; + +const counts: PrTabCounts = { + conversation: 0, + commits: 0, + checks: 1, + changes: 0, +}; + +function renderChecksTab(checksStatus: PrChecksStatus) { + render( + undefined} + counts={counts} + checksStatus={checksStatus} + />, + ); + return screen.getByRole("tab", { name: /Checks/ }).querySelector("svg"); +} + +describe("PrTabsPill", () => { + it("colors the Checks tab icon green when checks pass", () => { + expect(renderChecksTab("SUCCESS")).toHaveClass("text-success"); + }); + + it("colors the Checks tab icon yellow when checks are pending", () => { + expect(renderChecksTab("PENDING")).toHaveClass("text-warning"); + }); + + it("colors the Checks tab icon red when checks fail", () => { + expect(renderChecksTab("FAILURE")).toHaveClass("text-danger"); + }); +}); diff --git a/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.tsx b/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.tsx index 260b557e..08768d0d 100644 --- a/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.tsx +++ b/src/renderer/views/PrReviewOverlay/parts/PrTabsPill.tsx @@ -1,5 +1,10 @@ import { FileDiff, GitCommit, MessageSquare, ShieldCheck } from "lucide-react"; import { LightballTabs, type LightballTab } from "@/renderer/components/common"; +import { + getChecksStatusTone, + type PrChecksStatus, + type PrChecksTone, +} from "@/renderer/utils/prStatus"; export type PrTabKey = "conversation" | "commits" | "checks" | "changes"; @@ -21,6 +26,12 @@ const TAB_DEFS: ReadonlyArray<{ { id: "changes", label: "Changes", icon: FileDiff }, ]; +const CHECKS_ICON_TONE_CLASS: Record = { + success: "text-success", + warning: "text-warning", + danger: "text-danger", +}; + function CountChip(props: { value: number; active: boolean }) { return ( void; counts: PrTabCounts; + checksStatus?: PrChecksStatus | undefined; className?: string; }) { - const { active, onChange, counts, className } = props; + const { active, onChange, counts, checksStatus, className } = props; + const checksTone = getChecksStatusTone(checksStatus); const tabs: ReadonlyArray> = TAB_DEFS.map((def) => { const count = counts[def.id]; const Icon = def.icon; + const iconToneClass = + def.id === "checks" && checksTone ? CHECKS_ICON_TONE_CLASS[checksTone] : ""; + const iconClassName = `size-3${iconToneClass ? ` ${iconToneClass}` : ""}`; return { id: def.id, label: def.label, - icon: , + icon: , ...(count > 0 ? { trailing: (isActive: boolean) => } : {}),