From 25d94971756fd3d40b3260302197e30c31fb7c94 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 20 Mar 2026 21:07:12 -0700 Subject: [PATCH 1/5] desktop: add review tab to changes sidebar --- .../routers/workspaces/utils/github/github.ts | 74 +++- .../routers/workspaces/utils/github/types.ts | 13 + .../RightSidebar/ChangesView/ChangesView.tsx | 115 ++++--- .../ChangesHeader/ChangesHeader.tsx | 50 +-- .../PRChecksStatus/PRChecksStatus.tsx | 103 ------ .../components/PRChecksStatus/index.ts | 1 - .../components/ReviewPanel/ReviewPanel.tsx | 316 ++++++++++++++++++ .../components/ReviewPanel/index.ts | 1 + packages/local-db/src/schema/zod.ts | 12 + 9 files changed, 494 insertions(+), 191 deletions(-) delete mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/PRChecksStatus/PRChecksStatus.tsx delete mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/PRChecksStatus/index.ts create mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsx create mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/index.ts diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index 7649ae7c3f3..24c6f22be79 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -1,4 +1,8 @@ -import type { CheckItem, GitHubStatus } from "@superset/local-db"; +import type { + CheckItem, + GitHubStatus, + PullRequestComment, +} from "@superset/local-db"; import { branchExistsOnRemote, getTrackingRemoteNameForWorktree } from "../git"; import { execGitWithShellPath } from "../git-client"; import { execWithShellEnv } from "../shell-env"; @@ -217,7 +221,7 @@ export function getPullRequestRepoArgs( } const PR_JSON_FIELDS = - "number,title,url,state,isDraft,mergedAt,additions,deletions,headRefOid,headRefName,reviewDecision,statusCheckRollup,reviewRequests"; + "number,title,url,state,isDraft,mergedAt,additions,deletions,headRefOid,headRefName,reviewDecision,statusCheckRollup,comments,reviewRequests"; async function getPRForBranch( worktreePath: string, @@ -408,10 +412,34 @@ function formatPRData(data: GHPRResponse): NonNullable { reviewDecision: mapReviewDecision(data.reviewDecision), checksStatus: computeChecksStatus(data.statusCheckRollup), checks: parseChecks(data.statusCheckRollup), + comments: parseComments(data.comments), requestedReviewers: parseReviewRequests(data.reviewRequests), }; } +function formatShortDuration(durationMs: number): string | undefined { + if (!Number.isFinite(durationMs) || durationMs < 0) { + return undefined; + } + + const totalSeconds = Math.max(1, Math.round(durationMs / 1000)); + if (totalSeconds < 60) { + return `${totalSeconds}s`; + } + + const totalMinutes = Math.round(totalSeconds / 60); + if (totalMinutes < 60) { + return `${totalMinutes}m`; + } + + const totalHours = Math.round(totalMinutes / 60); + if (totalHours < 24) { + return `${totalHours}h`; + } + + return `${Math.round(totalHours / 24)}d`; +} + function parseReviewRequests( requests: GHPRResponse["reviewRequests"], ): string[] { @@ -466,10 +494,50 @@ function parseChecks(rollup: GHPRResponse["statusCheckRollup"]): CheckItem[] { status = "pending"; } - return { name, status, url }; + let durationText: string | undefined; + if (ctx.startedAt) { + const startedAt = Date.parse(ctx.startedAt); + const completedAt = ctx.completedAt + ? Date.parse(ctx.completedAt) + : Date.now(); + if (!Number.isNaN(startedAt) && !Number.isNaN(completedAt)) { + durationText = formatShortDuration(completedAt - startedAt); + } + } + + return { name, status, url, durationText }; }); } +function parseComments( + comments: GHPRResponse["comments"], +): PullRequestComment[] { + if (!comments || comments.length === 0) { + return []; + } + + return comments + .map((comment, index) => { + const createdAt = comment.createdAt + ? new Date(comment.createdAt).getTime() + : undefined; + const authorLogin = comment.author?.login || "github"; + const id = + comment.id || + comment.url || + `${authorLogin}-${createdAt ?? "unknown"}-${index}`; + + return { + id, + authorLogin, + body: comment.body ?? "", + createdAt: Number.isNaN(createdAt) ? undefined : createdAt, + url: comment.url, + }; + }) + .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); +} + /** * Low-level helper: query deployments matching the given params and return * the environment_url of the first successful deployment. Status lookups diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts index cb26cabfa93..0f2c7425088 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts @@ -32,6 +32,18 @@ export const GHReviewRequestSchema = z.object({ type: z.enum(["User", "Team"]).optional(), }); +export const GHCommentAuthorSchema = z.object({ + login: z.string().optional(), +}); + +export const GHCommentSchema = z.object({ + id: z.string().optional(), + author: GHCommentAuthorSchema.nullable().optional(), + body: z.string().optional(), + createdAt: z.string().optional(), + url: z.string().optional(), +}); + export const GHPRResponseSchema = z.object({ number: z.number(), title: z.string(), @@ -48,6 +60,7 @@ export const GHPRResponseSchema = z.object({ .nullable(), // statusCheckRollup is an array directly, not { contexts: [...] } statusCheckRollup: z.array(GHCheckContextSchema).nullable(), + comments: z.array(GHCommentSchema).nullable().optional(), reviewRequests: z.array(GHReviewRequestSchema).nullable().optional(), }); diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx index 0b7a042fd43..4ffbc27bb71 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx @@ -8,6 +8,7 @@ import { } from "@superset/ui/alert-dialog"; import { Button } from "@superset/ui/button"; import { toast } from "@superset/ui/sonner"; +import { Tabs, TabsContent, TabsList, TabsTrigger } from "@superset/ui/tabs"; import { useParams } from "@tanstack/react-router"; import { useEffect, useMemo, useRef, useState } from "react"; import { electronTrpc } from "renderer/lib/electron-trpc"; @@ -25,7 +26,7 @@ import type { FileSystemChangeEvent } from "shared/file-tree-types"; import { CategorySection } from "./components/CategorySection"; import { ChangesHeader } from "./components/ChangesHeader"; import { CommitInput } from "./components/CommitInput"; -import { PRChecksStatus } from "./components/PRChecksStatus"; +import { ReviewPanel } from "./components/ReviewPanel"; import { useOrderedSections } from "./hooks"; import { getPRActionState, shouldAutoCreatePRAfterPublish } from "./utils"; @@ -46,6 +47,8 @@ interface PendingChangesRefresh { invalidateSelectedFile: boolean; } +type ChangesSidebarTab = "diffs" | "review"; + function eventTargetsSelectedFile( event: FileSystemChangeEvent, selectedAbsolutePath: string | null, @@ -256,6 +259,7 @@ export function ChangesView({ const [showDiscardUnstagedDialog, setShowDiscardUnstagedDialog] = useState(false); const [showDiscardStagedDialog, setShowDiscardStagedDialog] = useState(false); + const [activeTab, setActiveTab] = useState("diffs"); const refreshTimerRef = useRef | null>(null); const pendingRefreshRef = useRef({ invalidateBranches: false, @@ -299,6 +303,7 @@ export function ChangesView({ // biome-ignore lint/correctness/useExhaustiveDependencies: reset on workspace change useEffect(() => { setExpandedCommits(new Set()); + setActiveTab("diffs"); }, [worktreePath]); useEffect(() => { @@ -648,6 +653,7 @@ export function ChangesView({ onRefresh={handleRefresh} viewMode={fileListViewMode} onViewModeChange={setFileListViewMode} + showViewModeToggle={activeTab === "diffs"} worktreePath={worktreePath} pr={githubStatus?.pr ?? null} isPRStatusLoading={isGitHubStatusLoading} @@ -664,47 +670,74 @@ export function ChangesView({ stashPopMutation.isPending } /> - -
- {githubStatus?.pr && } - -
- - {!hasChanges ? ( -
- No changes detected -
- ) : ( -
- {orderedSections - .filter((section) => section.count > 0) - .map((section) => ( - - {section.content} - - ))} + setActiveTab(value as ChangesSidebarTab)} + className="flex flex-1 min-h-0 flex-col gap-0" + > +
+ + + Diffs + + + Review + +
- )} + + +
+ +
+ + {!hasChanges ? ( +
+ No changes detected +
+ ) : ( +
+ {orderedSections + .filter((section) => section.count > 0) + .map((section) => ( + + {section.content} + + ))} +
+ )} + + + + + + void; viewMode: ChangesViewMode; onViewModeChange: (mode: ChangesViewMode) => void; + showViewModeToggle?: boolean; worktreePath: string; pr: GitHubStatus["pr"] | null; isPRStatusLoading: boolean; @@ -238,47 +239,11 @@ function RefreshButton({ onRefresh }: { onRefresh: () => void }) { ); } -const reviewTagStyles = { - approved: - "border border-emerald-500/20 bg-emerald-500/10 text-emerald-700 dark:text-emerald-300", - changes_requested: - "border border-red-500/20 bg-red-500/10 text-red-700 dark:text-red-300", - pending: - "border border-amber-500/20 bg-amber-500/10 text-amber-700 dark:text-amber-300", -} as const; - -const reviewTagLabels = { - approved: "Approved", - changes_requested: "Changes req.", - pending: "Review pending", -} as const; - -function ReviewTag({ - status, - requestedReviewers, -}: { - status: "approved" | "changes_requested" | "pending"; - requestedReviewers?: string[]; -}) { - const label = - status === "pending" && requestedReviewers && requestedReviewers.length > 0 - ? `Awaiting ${requestedReviewers.join(", ")}` - : reviewTagLabels[status]; - - return ( - - {label} - - ); -} - export function ChangesHeader({ onRefresh, viewMode, onViewModeChange, + showViewModeToggle = true, worktreePath, pr, isPRStatusLoading, @@ -298,14 +263,13 @@ export function ChangesHeader({ onStashPop={onStashPop} isPending={isStashPending} /> - - - {pr && pr.state === "open" && ( - )} + ; -} - -const checkIconConfig = { - success: { - icon: LuCheck, - className: "text-emerald-600 dark:text-emerald-400", - }, - failure: { icon: LuX, className: "text-red-600 dark:text-red-400" }, - pending: { - icon: LuLoaderCircle, - className: "text-amber-600 dark:text-amber-400", - }, - skipped: { icon: LuMinus, className: "text-muted-foreground" }, - cancelled: { icon: LuMinus, className: "text-muted-foreground" }, -} as const; - -function CheckRow({ check }: { check: CheckItem }) { - const { icon: Icon, className } = checkIconConfig[check.status]; - - const content = ( - - - {check.name} - - ); - - if (check.url) { - return ( - - {content} - - ); - } - - return
{content}
; -} - -export function PRChecksStatus({ pr }: PRChecksStatusProps) { - const [checksExpanded, setChecksExpanded] = useState(false); - - if (pr.state !== "open") return null; - - const relevantChecks = pr.checks.filter( - (c) => c.status !== "skipped" && c.status !== "cancelled", - ); - const passing = relevantChecks.filter((c) => c.status === "success").length; - const total = relevantChecks.length; - - if (total === 0) return null; - - const checksIcon = - checkIconConfig[pr.checksStatus === "none" ? "pending" : pr.checksStatus]; - - return ( -
- - - {checksExpanded && ( -
- {relevantChecks.map((check) => ( - - ))} -
- )} -
- ); -} diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/PRChecksStatus/index.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/PRChecksStatus/index.ts deleted file mode 100644 index 1e46e5df80e..00000000000 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/PRChecksStatus/index.ts +++ /dev/null @@ -1 +0,0 @@ -export { PRChecksStatus } from "./PRChecksStatus"; diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsx new file mode 100644 index 00000000000..2be787d4dff --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsx @@ -0,0 +1,316 @@ +import type { GitHubStatus } from "@superset/local-db"; +import { Avatar, AvatarFallback } from "@superset/ui/avatar"; +import { cn } from "@superset/ui/utils"; +import { + LuArrowUpRight, + LuCheck, + LuLoaderCircle, + LuMessageSquareText, + LuMinus, + LuX, +} from "react-icons/lu"; +import { PRIcon } from "renderer/screens/main/components/PRIcon"; + +interface ReviewPanelProps { + pr: GitHubStatus["pr"] | null; +} + +const reviewDecisionConfig = { + approved: { + label: "Approved", + className: + "border border-emerald-500/20 bg-emerald-500/10 text-emerald-700 dark:text-emerald-300", + }, + changes_requested: { + label: "Changes requested", + className: + "border border-red-500/20 bg-red-500/10 text-red-700 dark:text-red-300", + }, + pending: { + label: "Review pending", + className: + "border border-amber-500/20 bg-amber-500/10 text-amber-700 dark:text-amber-300", + }, +} as const; + +const checkIconConfig = { + success: { + icon: LuCheck, + className: "text-emerald-600 dark:text-emerald-400", + label: "Passed", + }, + failure: { + icon: LuX, + className: "text-red-600 dark:text-red-400", + label: "Failed", + }, + pending: { + icon: LuLoaderCircle, + className: "text-amber-600 dark:text-amber-400", + label: "Pending", + }, + skipped: { + icon: LuMinus, + className: "text-muted-foreground", + label: "Skipped", + }, + cancelled: { + icon: LuMinus, + className: "text-muted-foreground", + label: "Cancelled", + }, +} as const; + +const checkSummaryIconConfig = { + success: checkIconConfig.success, + failure: checkIconConfig.failure, + pending: checkIconConfig.pending, + none: { + icon: LuMinus, + className: "text-muted-foreground", + label: "No checks", + }, +} as const; + +const prStateLabel = { + open: "Open", + draft: "Draft", + merged: "Merged", + closed: "Closed", +} as const; + +function getCommentPreview(body: string): string { + return ( + body + .split(/\r?\n/) + .map((line) => line.trim()) + .find(Boolean) + ?.replace(/\s+/g, " ") ?? "No preview available" + ); +} + +function getAvatarFallback(authorLogin: string): string { + return authorLogin.slice(0, 2).toUpperCase(); +} + +function formatShortAge(timestamp?: number): string | null { + if (!timestamp || Number.isNaN(timestamp)) { + return null; + } + + const deltaMs = Math.max(0, Date.now() - timestamp); + const deltaSeconds = Math.round(deltaMs / 1000); + + if (deltaSeconds < 60) { + return `${Math.max(1, deltaSeconds)}s`; + } + + const deltaMinutes = Math.round(deltaSeconds / 60); + if (deltaMinutes < 60) { + return `${deltaMinutes}m`; + } + + const deltaHours = Math.round(deltaMinutes / 60); + if (deltaHours < 24) { + return `${deltaHours}h`; + } + + return `${Math.round(deltaHours / 24)}d`; +} + +export function ReviewPanel({ pr }: ReviewPanelProps) { + if (!pr) { + return ( +
+ Open a pull request to view review status, checks, and comments. +
+ ); + } + + const requestedReviewers = pr.requestedReviewers ?? []; + const reviewLabel = + pr.reviewDecision === "pending" && requestedReviewers.length > 0 + ? `Awaiting ${requestedReviewers.join(", ")}` + : reviewDecisionConfig[pr.reviewDecision].label; + + const relevantChecks = pr.checks.filter( + (check) => check.status !== "skipped" && check.status !== "cancelled", + ); + const passingChecks = relevantChecks.filter( + (check) => check.status === "success", + ).length; + const checksSummary = + relevantChecks.length > 0 + ? `${passingChecks}/${relevantChecks.length} checks passing` + : "No checks reported"; + const checksStatusConfig = checkSummaryIconConfig[pr.checksStatus]; + const ChecksStatusIcon = checksStatusConfig.icon; + const comments = pr.comments ?? []; + + return ( +
+
+
+ + + {pr.title} + + + #{pr.number} + +
+ +
+ + {reviewDecisionConfig[pr.reviewDecision].label} + + + {requestedReviewers.length > 0 + ? reviewLabel + : prStateLabel[pr.state]} + +
+
+ +
+
+ Checks +
+ + {checksSummary} +
+
+
+ +
+ {relevantChecks.length === 0 ? ( +
+ No active checks reported for this pull request yet. +
+ ) : ( + relevantChecks.map((check) => { + const { icon: CheckIcon, className } = + checkIconConfig[check.status]; + + return check.url ? ( + +
+ + {check.name} + {check.durationText && ( + + {check.durationText} + + )} + +
+ + ) : ( +
+ + {check.name} + {check.durationText && ( + + {check.durationText} + + )} +
+ ); + }) + )} +
+ +
+
+
+ + Comments +
+ {comments.length} +
+
+ + +
+ ); +} diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/index.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/index.ts new file mode 100644 index 00000000000..a5ab0593815 --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/index.ts @@ -0,0 +1 @@ +export { ReviewPanel } from "./ReviewPanel"; diff --git a/packages/local-db/src/schema/zod.ts b/packages/local-db/src/schema/zod.ts index 4bdef9d4c5e..ea21d71ad7d 100644 --- a/packages/local-db/src/schema/zod.ts +++ b/packages/local-db/src/schema/zod.ts @@ -20,10 +20,21 @@ export const checkItemSchema = z.object({ name: z.string(), status: z.enum(["success", "failure", "pending", "skipped", "cancelled"]), url: z.string().optional(), + durationText: z.string().optional(), }); export type CheckItem = z.infer; +export const pullRequestCommentSchema = z.object({ + id: z.string(), + authorLogin: z.string(), + body: z.string(), + createdAt: z.number().optional(), + url: z.string().optional(), +}); + +export type PullRequestComment = z.infer; + /** * GitHub PR status */ @@ -40,6 +51,7 @@ export const gitHubStatusSchema = z.object({ reviewDecision: z.enum(["approved", "changes_requested", "pending"]), checksStatus: z.enum(["success", "failure", "pending", "none"]), checks: z.array(checkItemSchema), + comments: z.array(pullRequestCommentSchema).optional(), requestedReviewers: z.array(z.string()).optional(), }) .nullable(), From 51ece03e148f65e99b9240658ca8f3e269b91dfd Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 21 Mar 2026 12:36:45 -0700 Subject: [PATCH 2/5] Saving --- .../workspaces/procedures/git-status.ts | 20 +- .../workspaces/utils/github/github.test.ts | 114 +++++- .../routers/workspaces/utils/github/github.ts | 223 +++++++++-- .../routers/workspaces/utils/github/index.ts | 1 + .../routers/workspaces/utils/github/types.ts | 20 + .../RightSidebar/ChangesView/ChangesView.tsx | 141 ++++--- .../components/ReviewPanel/ReviewPanel.tsx | 372 ++++++++++++------ .../RightSidebar/headerTabStyles.ts | 29 ++ .../WorkspaceView/RightSidebar/index.tsx | 19 +- packages/local-db/src/schema/zod.ts | 4 + 10 files changed, 742 insertions(+), 201 deletions(-) create mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts index c07ff59097c..25e761eec20 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts @@ -17,7 +17,7 @@ import { listExternalWorktrees, refreshDefaultBranch, } from "../utils/git"; -import { fetchGitHubPRStatus } from "../utils/github"; +import { fetchGitHubPRComments, fetchGitHubPRStatus } from "../utils/github"; export const createGitStatusProcedures = () => { return router({ @@ -130,6 +130,24 @@ export const createGitStatusProcedures = () => { return freshStatus; }), + getGitHubPRComments: publicProcedure + .input(z.object({ workspaceId: z.string() })) + .query(async ({ input }) => { + const workspace = getWorkspace(input.workspaceId); + if (!workspace) { + return []; + } + + const worktree = workspace.worktreeId + ? getWorktree(workspace.worktreeId) + : null; + if (!worktree) { + return []; + } + + return fetchGitHubPRComments(worktree.path); + }), + getWorktreeInfo: publicProcedure .input(z.object({ workspaceId: z.string() })) .query(({ input }) => { diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts index ec59f9cfd10..9aad9d6ce57 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts @@ -1,5 +1,11 @@ import { describe, expect, test } from "bun:test"; -import { branchMatchesPR, getPullRequestRepoArgs } from "./github"; +import { + branchMatchesPR, + getPullRequestRepoArgs, + mergePullRequestComments, + parseConversationCommentsResponse, + parseReviewCommentsResponse, +} from "./github"; describe("branchMatchesPR", () => { test("matches same-repo branch exactly", () => { @@ -57,3 +63,109 @@ describe("getPullRequestRepoArgs", () => { ).toEqual([]); }); }); + +describe("parseReviewCommentsResponse", () => { + test("normalizes inline review comments with file metadata", () => { + expect( + parseReviewCommentsResponse([ + { + id: 42, + user: { + login: "octocat", + avatar_url: "https://avatars.githubusercontent.com/u/1?v=4", + }, + body: "Please rename this helper.", + created_at: "2026-03-21T04:19:41Z", + html_url: + "https://github.com/superset-sh/superset/pull/2681#discussion_r42", + path: "apps/desktop/src/file.ts", + line: 19, + }, + ]), + ).toEqual([ + { + id: "review-42", + authorLogin: "octocat", + avatarUrl: "https://avatars.githubusercontent.com/u/1?v=4", + body: "Please rename this helper.", + createdAt: new Date("2026-03-21T04:19:41Z").getTime(), + url: "https://github.com/superset-sh/superset/pull/2681#discussion_r42", + kind: "review", + path: "apps/desktop/src/file.ts", + line: 19, + }, + ]); + }); +}); + +describe("parseConversationCommentsResponse", () => { + test("normalizes top-level PR conversation comments", () => { + expect( + parseConversationCommentsResponse([ + { + id: 7, + user: { + login: "hubot", + avatar_url: "https://avatars.githubusercontent.com/u/2?v=4", + }, + body: "Looks good overall.", + created_at: "2026-03-21T04:08:13Z", + html_url: + "https://github.com/superset-sh/superset/pull/2681#issuecomment-7", + }, + ]), + ).toEqual([ + { + id: "conversation-7", + authorLogin: "hubot", + avatarUrl: "https://avatars.githubusercontent.com/u/2?v=4", + body: "Looks good overall.", + createdAt: new Date("2026-03-21T04:08:13Z").getTime(), + url: "https://github.com/superset-sh/superset/pull/2681#issuecomment-7", + kind: "conversation", + }, + ]); + }); +}); + +describe("mergePullRequestComments", () => { + test("sorts mixed comment kinds by recency", () => { + expect( + mergePullRequestComments( + [ + { + id: "review-42", + authorLogin: "octocat", + body: "Inline note", + createdAt: 200, + kind: "review", + }, + ], + [ + { + id: "conversation-7", + authorLogin: "hubot", + body: "Top-level note", + createdAt: 100, + kind: "conversation", + }, + ], + ), + ).toEqual([ + { + id: "review-42", + authorLogin: "octocat", + body: "Inline note", + createdAt: 200, + kind: "review", + }, + { + id: "conversation-7", + authorLogin: "hubot", + body: "Top-level note", + createdAt: 100, + kind: "conversation", + }, + ]); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index 24c6f22be79..7210474bff9 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -9,17 +9,25 @@ import { execWithShellEnv } from "../shell-env"; import { GHDeploymentSchema, GHDeploymentStatusSchema, + GHIssueCommentSchema, type GHPRResponse, GHPRResponseSchema, GHRepoResponseSchema, + GHReviewCommentSchema, type RepoContext, } from "./types"; const cache = new Map(); const CACHE_TTL_MS = 10_000; +const commentsCache = new Map< + string, + { data: PullRequestComment[]; timestamp: number } +>(); +const COMMENTS_CACHE_TTL_MS = 30_000; export function clearGitHubStatusCacheForWorktree(worktreePath: string): void { cache.delete(worktreePath); + commentsCache.delete(worktreePath); } /** @@ -92,6 +100,66 @@ export async function fetchGitHubPRStatus( } } +export async function fetchGitHubPRComments( + worktreePath: string, +): Promise { + const cached = commentsCache.get(worktreePath); + if (cached && Date.now() - cached.timestamp < COMMENTS_CACHE_TTL_MS) { + return cached.data; + } + + try { + const repoContext = await getRepoContext(worktreePath); + if (!repoContext) { + return []; + } + + const [{ stdout: branchOutput }, { stdout: shaOutput }] = await Promise.all( + [ + execGitWithShellPath(["rev-parse", "--abbrev-ref", "HEAD"], { + cwd: worktreePath, + }), + execGitWithShellPath(["rev-parse", "HEAD"], { cwd: worktreePath }), + ], + ); + const branchName = branchOutput.trim(); + const headSha = shaOutput.trim(); + const prInfo = await getPRForBranch( + worktreePath, + branchName, + repoContext, + headSha, + ); + if (!prInfo) { + return []; + } + + const targetUrl = repoContext.isFork + ? repoContext.upstreamUrl + : repoContext.repoUrl; + const nwo = extractNwoFromUrl(targetUrl); + if (!nwo) { + return []; + } + + const [reviewComments, conversationComments] = await Promise.all([ + fetchReviewCommentsForPR(worktreePath, nwo, prInfo.number), + fetchConversationCommentsForPR(worktreePath, nwo, prInfo.number), + ]); + const comments = mergePullRequestComments( + reviewComments, + conversationComments, + ); + commentsCache.set(worktreePath, { + data: comments, + timestamp: Date.now(), + }); + return comments; + } catch { + return []; + } +} + const repoContextCache = new Map< string, { data: RepoContext; timestamp: number } @@ -221,7 +289,7 @@ export function getPullRequestRepoArgs( } const PR_JSON_FIELDS = - "number,title,url,state,isDraft,mergedAt,additions,deletions,headRefOid,headRefName,reviewDecision,statusCheckRollup,comments,reviewRequests"; + "number,title,url,state,isDraft,mergedAt,additions,deletions,headRefOid,headRefName,reviewDecision,statusCheckRollup,reviewRequests"; async function getPRForBranch( worktreePath: string, @@ -412,7 +480,6 @@ function formatPRData(data: GHPRResponse): NonNullable { reviewDecision: mapReviewDecision(data.reviewDecision), checksStatus: computeChecksStatus(data.statusCheckRollup), checks: parseChecks(data.statusCheckRollup), - comments: parseComments(data.comments), requestedReviewers: parseReviewRequests(data.reviewRequests), }; } @@ -509,35 +576,143 @@ function parseChecks(rollup: GHPRResponse["statusCheckRollup"]): CheckItem[] { }); } -function parseComments( - comments: GHPRResponse["comments"], -): PullRequestComment[] { - if (!comments || comments.length === 0) { +function parseTimestamp(value?: string): number | undefined { + if (!value) { + return undefined; + } + + const timestamp = new Date(value).getTime(); + return Number.isNaN(timestamp) ? undefined : timestamp; +} + +function parseApiArray(stdout: string): unknown[] { + const trimmed = stdout.trim(); + if (!trimmed || trimmed === "null") { return []; } - return comments - .map((comment, index) => { - const createdAt = comment.createdAt - ? new Date(comment.createdAt).getTime() - : undefined; - const authorLogin = comment.author?.login || "github"; - const id = - comment.id || - comment.url || - `${authorLogin}-${createdAt ?? "unknown"}-${index}`; - - return { - id, - authorLogin, - body: comment.body ?? "", - createdAt: Number.isNaN(createdAt) ? undefined : createdAt, - url: comment.url, - }; + try { + const raw = JSON.parse(trimmed); + return Array.isArray(raw) ? raw : []; + } catch (error) { + console.warn( + "[GitHub] Failed to parse API array response:", + error instanceof Error ? error.message : String(error), + ); + return []; + } +} + +async function fetchReviewCommentsForPR( + worktreePath: string, + nwo: string, + prNumber: number, +): Promise { + const { stdout } = await execWithShellEnv( + "gh", + ["api", `repos/${nwo}/pulls/${prNumber}/comments?per_page=100`], + { cwd: worktreePath }, + ); + return parseReviewCommentsResponse(parseApiArray(stdout)); +} + +async function fetchConversationCommentsForPR( + worktreePath: string, + nwo: string, + prNumber: number, +): Promise { + const { stdout } = await execWithShellEnv( + "gh", + ["api", `repos/${nwo}/issues/${prNumber}/comments?per_page=100`], + { cwd: worktreePath }, + ); + return parseConversationCommentsResponse(parseApiArray(stdout)); +} + +export function parseReviewCommentsResponse( + raw: unknown[], +): PullRequestComment[] { + return raw + .flatMap((item) => { + const result = GHReviewCommentSchema.safeParse(item); + if (!result.success) { + return []; + } + + const comment = result.data; + const body = comment.body?.trim(); + if (!body) { + return []; + } + + return [ + { + id: `review-${comment.id}`, + authorLogin: comment.user?.login || "github", + ...(comment.user?.avatar_url + ? { avatarUrl: comment.user.avatar_url } + : {}), + body, + createdAt: parseTimestamp(comment.created_at), + url: comment.html_url, + kind: "review" as const, + path: comment.path, + line: comment.line ?? comment.original_line ?? undefined, + }, + ]; }) .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); } +export function parseConversationCommentsResponse( + raw: unknown[], +): PullRequestComment[] { + return raw + .flatMap((item) => { + const result = GHIssueCommentSchema.safeParse(item); + if (!result.success) { + return []; + } + + const comment = result.data; + const body = comment.body?.trim(); + if (!body) { + return []; + } + + return [ + { + id: `conversation-${comment.id}`, + authorLogin: comment.user?.login || "github", + ...(comment.user?.avatar_url + ? { avatarUrl: comment.user.avatar_url } + : {}), + body, + createdAt: parseTimestamp(comment.created_at), + url: comment.html_url, + kind: "conversation" as const, + }, + ]; + }) + .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); +} + +export function mergePullRequestComments( + ...commentGroups: PullRequestComment[][] +): PullRequestComment[] { + const commentsById = new Map(); + + for (const group of commentGroups) { + for (const comment of group) { + commentsById.set(comment.id, comment); + } + } + + return [...commentsById.values()].sort( + (a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0), + ); +} + /** * Low-level helper: query deployments matching the given params and return * the environment_url of the first successful deployment. Status lookups diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts index a3fca6bcd0b..4fea9921aaf 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts @@ -1,4 +1,5 @@ export { clearGitHubStatusCacheForWorktree, + fetchGitHubPRComments, fetchGitHubPRStatus, } from "./github"; diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts index 0f2c7425088..469a4b8771b 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts @@ -34,6 +34,7 @@ export const GHReviewRequestSchema = z.object({ export const GHCommentAuthorSchema = z.object({ login: z.string().optional(), + avatar_url: z.string().optional(), }); export const GHCommentSchema = z.object({ @@ -44,6 +45,25 @@ export const GHCommentSchema = z.object({ url: z.string().optional(), }); +export const GHReviewCommentSchema = z.object({ + id: z.number(), + user: GHCommentAuthorSchema.nullable().optional(), + body: z.string().optional(), + created_at: z.string().optional(), + html_url: z.string().optional(), + path: z.string().optional(), + line: z.number().nullable().optional(), + original_line: z.number().nullable().optional(), +}); + +export const GHIssueCommentSchema = z.object({ + id: z.number(), + user: GHCommentAuthorSchema.nullable().optional(), + body: z.string().optional(), + created_at: z.string().optional(), + html_url: z.string().optional(), +}); + export const GHPRResponseSchema = z.object({ number: z.number(), title: z.string(), diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx index 4ffbc27bb71..09ca0dbbbd6 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx @@ -1,3 +1,4 @@ +import * as TabsPrimitive from "@radix-ui/react-tabs"; import { AlertDialog, AlertDialogContent, @@ -8,7 +9,6 @@ import { } from "@superset/ui/alert-dialog"; import { Button } from "@superset/ui/button"; import { toast } from "@superset/ui/sonner"; -import { Tabs, TabsContent, TabsList, TabsTrigger } from "@superset/ui/tabs"; import { useParams } from "@tanstack/react-router"; import { useEffect, useMemo, useRef, useState } from "react"; import { electronTrpc } from "renderer/lib/electron-trpc"; @@ -23,6 +23,7 @@ import { } from "shared/absolute-paths"; import type { ChangeCategory, ChangedFile } from "shared/changes-types"; import type { FileSystemChangeEvent } from "shared/file-tree-types"; +import { sidebarHeaderTabTriggerClassName } from "../headerTabStyles"; import { CategorySection } from "./components/CategorySection"; import { ChangesHeader } from "./components/ChangesHeader"; import { CommitInput } from "./components/CommitInput"; @@ -112,17 +113,6 @@ export function ChangesView({ }, ); - useBranchSyncInvalidation({ - gitBranch: status?.branch ?? branchData?.currentBranch ?? undefined, - workspaceBranch: workspace?.branch, - workspaceId: workspaceId ?? "", - }); - - const handleRefresh = () => { - refetch(); - refetchGithubStatus(); - }; - const stageAllMutation = electronTrpc.changes.stageAll.useMutation({ onSuccess: () => refetch(), onError: (error) => { @@ -265,6 +255,32 @@ export function ChangesView({ invalidateBranches: false, invalidateSelectedFile: false, }); + const { + data: githubComments = [], + isLoading: isGitHubCommentsLoading, + refetch: refetchGitHubComments, + } = electronTrpc.workspaces.getGitHubPRComments.useQuery( + { workspaceId: workspaceId ?? "" }, + { + enabled: !!workspaceId && isActive && !!githubStatus?.pr, + refetchInterval: isActive && githubStatus?.pr ? 30_000 : false, + refetchOnWindowFocus: isActive, + }, + ); + + useBranchSyncInvalidation({ + gitBranch: status?.branch ?? branchData?.currentBranch ?? undefined, + workspaceBranch: workspace?.branch, + workspaceId: workspaceId ?? "", + }); + + const handleRefresh = () => { + refetch(); + refetchGithubStatus(); + if (githubStatus?.pr) { + refetchGitHubComments(); + } + }; const handleDiscard = (file: ChangedFile) => { if (!worktreePath) return; @@ -647,49 +663,66 @@ export function ChangesView({ ); } + const againstMainCount = status.againstBase.length; + const reviewCommentCount = githubStatus?.pr ? githubComments.length : 0; + return (
- stashMutation.mutate({ worktreePath })} - onStashIncludeUntracked={() => - stashIncludeUntrackedMutation.mutate({ worktreePath }) - } - onStashPop={() => stashPopMutation.mutate({ worktreePath })} - isStashPending={ - stashMutation.isPending || - stashIncludeUntrackedMutation.isPending || - stashPopMutation.isPending - } - /> - setActiveTab(value as ChangesSidebarTab)} className="flex flex-1 min-h-0 flex-col gap-0" > -
- - - Diffs - - - Review - - +
+ + + Diffs + + {againstMainCount} + + + + Review + + {reviewCommentCount} + + +
- +
+ stashMutation.mutate({ worktreePath })} + onStashIncludeUntracked={() => + stashIncludeUntrackedMutation.mutate({ worktreePath }) + } + onStashPop={() => stashPopMutation.mutate({ worktreePath })} + isStashPending={ + stashMutation.isPending || + stashIncludeUntrackedMutation.isPending || + stashPopMutation.isPending + } + /> +
)} - + - - - - + + + + /g, "\n") .split(/\r?\n/) .map((line) => line.trim()) .find(Boolean) + ?.replace(/^[-*+>]\s*/, "") ?.replace(/\s+/g, " ") ?? "No preview available" ); } @@ -118,7 +131,66 @@ function formatShortAge(timestamp?: number): string | null { return `${Math.round(deltaHours / 24)}d`; } -export function ReviewPanel({ pr }: ReviewPanelProps) { +function getCommentLocation(comment: PullRequestComment): string | null { + if (comment.path) { + return comment.line ? `${comment.path}:${comment.line}` : comment.path; + } + + return comment.kind === "conversation" ? "Conversation" : null; +} + +function getCommentKindLabel(comment: PullRequestComment): string { + return comment.kind === "review" ? "Review" : "Comment"; +} + +export function ReviewPanel({ + pr, + comments = [], + isLoading = false, + isCommentsLoading = false, +}: ReviewPanelProps) { + const [checksOpen, setChecksOpen] = useState(true); + const [commentsOpen, setCommentsOpen] = useState(true); + + if (isLoading && !pr) { + return ( +
+
+
+ + + +
+
+ + +
+
+
+
+ + +
+
+ + +
+
+
+
+ + +
+
+ + + +
+
+
+ ); + } + if (!pr) { return (
@@ -143,30 +215,30 @@ export function ReviewPanel({ pr }: ReviewPanelProps) { relevantChecks.length > 0 ? `${passingChecks}/${relevantChecks.length} checks passing` : "No checks reported"; - const checksStatusConfig = checkSummaryIconConfig[pr.checksStatus]; + const checksStatus = relevantChecks.length > 0 ? pr.checksStatus : "none"; + const checksStatusConfig = checkSummaryIconConfig[checksStatus]; const ChecksStatusIcon = checksStatusConfig.icon; - const comments = pr.comments ?? []; return ( -
-
-
+
+
+
{pr.title} - + #{pr.number}
-
+
{reviewDecisionConfig[pr.reviewDecision].label} - + {requestedReviewers.length > 0 ? reviewLabel : prStateLabel[pr.state]} @@ -183,45 +255,84 @@ export function ReviewPanel({ pr }: ReviewPanelProps) {
-
-
- Checks + + +
+ + Checks + + {relevantChecks.length} + +
- {checksSummary} -
-
-
- -
- {relevantChecks.length === 0 ? ( -
- No active checks reported for this pull request yet. + + {checksSummary} +
- ) : ( - relevantChecks.map((check) => { - const { icon: CheckIcon, className } = - checkIconConfig[check.status]; + + + {relevantChecks.length === 0 ? ( +
+ No active checks reported for this pull request yet. +
+ ) : ( + relevantChecks.map((check) => { + const { icon: CheckIcon, className } = + checkIconConfig[check.status]; - return check.url ? ( - -
+ return check.url ? ( + +
+ + + {check.name} + + {check.durationText && ( + + {check.durationText} + + )} + +
+ + ) : ( +
{check.name} {check.durationText && ( - + {check.durationText} )} -
- - ) : ( -
- - {check.name} - {check.durationText && ( - - {check.durationText} - - )} -
- ); - }) - )} -
+ ); + }) + )} + + -
-
-
- - Comments -
- {comments.length} + +
+ + + + Comments + + {isCommentsLoading ? "..." : comments.length} + +
-
+ +
+ {isCommentsLoading ? ( +
+ + + +
+ ) : comments.length === 0 ? ( +
+ No comments yet. +
+ ) : ( + comments.map((comment) => { + const location = getCommentLocation(comment); + const age = formatShortAge(comment.createdAt); + const content = ( + <> + + {comment.avatarUrl ? ( + + ) : null} + + {getAvatarFallback(comment.authorLogin)} + + +
+
+ + {comment.authorLogin} + + + {getCommentKindLabel(comment)} + + {age && ( + + {age} + + )} +
+ {location && ( +

+ {location} +

+ )} +

+ {getCommentPreview(comment.body)} +

+
+ {comment.url ? ( + + ) : null} + + ); + + const baseClassName = + "flex items-start gap-2 rounded-sm px-1.5 py-1.5"; -
- {comments.length === 0 ? ( -
- No comments yet. + return comment.url ? ( + + {content} + + ) : ( +
+ {content} +
+ ); + }) + )}
- ) : ( - comments.map((comment) => ( - - - - {getAvatarFallback(comment.authorLogin)} - - -
-
- - {comment.authorLogin} - - {formatShortAge(comment.createdAt) && ( - - {formatShortAge(comment.createdAt)} - - )} -
-

- {getCommentPreview(comment.body)} -

-
- - - )) - )} -
+ +
); } diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts new file mode 100644 index 00000000000..26409fb3417 --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts @@ -0,0 +1,29 @@ +import { cn } from "@superset/ui/utils"; + +const SIDEBAR_HEADER_TAB_ACTIVE_CLASS_NAME = "text-foreground bg-border/30"; +const SIDEBAR_HEADER_TAB_INACTIVE_CLASS_NAME = + "text-muted-foreground/70 hover:text-muted-foreground hover:bg-tertiary/20"; + +export function getSidebarHeaderTabButtonClassName({ + isActive, + compact = false, +}: { + isActive: boolean; + compact?: boolean; +}) { + return cn( + "h-full shrink-0 transition-all", + compact + ? "flex w-10 items-center justify-center" + : "flex items-center gap-2 px-3 text-sm", + isActive + ? SIDEBAR_HEADER_TAB_ACTIVE_CLASS_NAME + : SIDEBAR_HEADER_TAB_INACTIVE_CLASS_NAME, + ); +} + +export const sidebarHeaderTabTriggerClassName = cn( + "flex h-full shrink-0 items-center gap-2 border-0 bg-transparent px-3 text-sm shadow-none transition-all outline-none", + "data-[state=active]:bg-border/30 data-[state=active]:text-foreground", + "data-[state=inactive]:text-muted-foreground/70 data-[state=inactive]:hover:bg-tertiary/20 data-[state=inactive]:hover:text-muted-foreground", +); diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx index 7ab8514ce48..4fd295c53d0 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/index.tsx @@ -1,6 +1,5 @@ import { Button } from "@superset/ui/button"; import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; -import { cn } from "@superset/ui/utils"; import { useParams } from "@tanstack/react-router"; import { useCallback } from "react"; import { @@ -23,6 +22,7 @@ import type { ChangeCategory, ChangedFile } from "shared/changes-types"; import { useScrollContext } from "../ChangesContent"; import { ChangesView } from "./ChangesView"; import { FilesView } from "./FilesView"; +import { getSidebarHeaderTabButtonClassName } from "./headerTabStyles"; function TabButton({ isActive, @@ -44,12 +44,10 @@ function TabButton({ @@ -65,12 +63,7 @@ function TabButton({ + )}
@@ -394,6 +523,8 @@ export function ReviewPanel({ comments.map((comment) => { const location = getCommentLocation(comment); const age = formatShortAge(comment.createdAt); + const copyKey = `comment-${comment.id}`; + const isCopied = copiedItemKey === copyKey; const content = ( <> @@ -426,32 +557,53 @@ export function ReviewPanel({ {location}

)} -

+

{getCommentPreview(comment.body)}

{comment.url ? ( - + ) : null} ); - const baseClassName = - "flex items-start gap-2 rounded-sm px-1.5 py-1.5"; - - return comment.url ? ( - - {content} - - ) : ( -
- {content} + {comment.url ? ( + + {content} + + ) : ( +
+ {content} +
+ )} +
); }) diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts index 26409fb3417..627cb742654 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts @@ -23,7 +23,7 @@ export function getSidebarHeaderTabButtonClassName({ } export const sidebarHeaderTabTriggerClassName = cn( - "flex h-full shrink-0 items-center gap-2 border-0 bg-transparent px-3 text-sm shadow-none transition-all outline-none", - "data-[state=active]:bg-border/30 data-[state=active]:text-foreground", + "flex h-full flex-none shrink-0 items-center gap-2 rounded-none border-0 bg-transparent px-3 text-sm font-normal shadow-none transition-all outline-none", + "data-[state=active]:bg-border/30 data-[state=active]:text-foreground data-[state=active]:shadow-none", "data-[state=inactive]:text-muted-foreground/70 data-[state=inactive]:hover:bg-tertiary/20 data-[state=inactive]:hover:text-muted-foreground", ); From 83435238215154d83b9a7b3aa95ae057cac592d6 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 21 Mar 2026 15:50:28 -0700 Subject: [PATCH 4/5] desktop: refine review tab GitHub fetching --- .../trpc/routers/changes/git-operations.ts | 4 +- .../workspaces/procedures/git-status.ts | 60 +- .../workspaces/utils/github/cache.test.ts | 68 +++ .../routers/workspaces/utils/github/cache.ts | 210 +++++++ .../workspaces/utils/github/comments.ts | 182 ++++++ .../workspaces/utils/github/github.test.ts | 17 + .../routers/workspaces/utils/github/github.ts | 532 ++++++++---------- .../routers/workspaces/utils/github/index.ts | 3 +- .../RightSidebar/ChangesView/ChangesView.tsx | 40 +- .../components/ReviewPanel/ReviewPanel.tsx | 329 +++-------- .../components/ReviewPanel/utils.ts | 180 ++++++ 11 files changed, 1075 insertions(+), 550 deletions(-) create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.test.ts create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts create mode 100644 apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/utils.ts diff --git a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts index 4361960f47a..f603f5b762e 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts @@ -7,7 +7,7 @@ import { getSimpleGitWithShellPath, } from "../workspaces/utils/git-client"; import { - clearGitHubStatusCacheForWorktree, + clearGitHubCachesForWorktree, getPullRequestRepoArgs, getRepoContext, } from "../workspaces/utils/github/github"; @@ -74,7 +74,7 @@ async function fetchCurrentBranch(git: SimpleGit): Promise { } function clearWorktreeStatusCaches(worktreePath: string): void { - clearGitHubStatusCacheForWorktree(worktreePath); + clearGitHubCachesForWorktree(worktreePath); clearStatusCacheForWorktree(worktreePath); } diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts index 25e761eec20..a21e718c4cb 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts @@ -1,4 +1,5 @@ import { existsSync } from "node:fs"; +import type { GitHubStatus } from "@superset/local-db"; import { workspaces, worktrees } from "@superset/local-db"; import { and, eq, isNull } from "drizzle-orm"; import { localDb } from "main/lib/local-db"; @@ -17,7 +18,52 @@ import { listExternalWorktrees, refreshDefaultBranch, } from "../utils/git"; -import { fetchGitHubPRComments, fetchGitHubPRStatus } from "../utils/github"; +import { + fetchGitHubPRComments, + fetchGitHubPRStatus, + type PullRequestCommentsTarget, +} from "../utils/github"; + +const gitHubPRCommentsInputSchema = z.object({ + workspaceId: z.string(), + prNumber: z.number().int().positive().optional(), + repoUrl: z.string().optional(), + upstreamUrl: z.string().optional(), + isFork: z.boolean().optional(), +}); + +function resolveCommentsPullRequestTarget({ + input, + githubStatus, +}: { + input: z.infer; + githubStatus: GitHubStatus | null | undefined; +}): PullRequestCommentsTarget | null { + const prNumber = input.prNumber ?? githubStatus?.pr?.number; + if (!prNumber) { + return null; + } + + const repoUrl = input.repoUrl ?? githubStatus?.repoUrl; + if (!repoUrl) { + return null; + } + + const upstreamUrl = + input.upstreamUrl ?? githubStatus?.upstreamUrl ?? githubStatus?.repoUrl; + if (!upstreamUrl) { + return null; + } + + return { + prNumber, + repoContext: { + repoUrl, + upstreamUrl, + isFork: input.isFork ?? githubStatus?.isFork ?? false, + }, + }; +} export const createGitStatusProcedures = () => { return router({ @@ -131,7 +177,7 @@ export const createGitStatusProcedures = () => { }), getGitHubPRComments: publicProcedure - .input(z.object({ workspaceId: z.string() })) + .input(gitHubPRCommentsInputSchema) .query(async ({ input }) => { const workspace = getWorkspace(input.workspaceId); if (!workspace) { @@ -145,7 +191,15 @@ export const createGitStatusProcedures = () => { return []; } - return fetchGitHubPRComments(worktree.path); + const cachedGitHubStatus = worktree.githubStatus ?? null; + + return fetchGitHubPRComments({ + worktreePath: worktree.path, + pullRequest: resolveCommentsPullRequestTarget({ + input, + githubStatus: cachedGitHubStatus, + }), + }); }), getWorktreeInfo: publicProcedure diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.test.ts new file mode 100644 index 00000000000..9b0003738de --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from "bun:test"; +import type { GitHubStatus, PullRequestComment } from "@superset/local-db"; +import { + clearGitHubCachesForWorktree, + getCachedGitHubStatus, + getCachedPullRequestComments, + getCachedRepoContext, + makePullRequestCommentsCacheKey, + setCachedGitHubStatus, + setCachedPullRequestComments, + setCachedRepoContext, +} from "./cache"; + +describe("clearGitHubCachesForWorktree", () => { + test("clears status, repo context, and comment entries for one worktree", () => { + const worktreePath = "/tmp/worktrees/review-cache-test"; + const otherWorktreePath = "/tmp/worktrees/review-cache-test-other"; + + const status: GitHubStatus = { + pr: null, + repoUrl: "https://github.com/superset-sh/superset", + upstreamUrl: "https://github.com/superset-sh/superset", + isFork: false, + branchExistsOnRemote: true, + lastRefreshed: Date.now(), + }; + const comments: PullRequestComment[] = [ + { + id: "review-1", + authorLogin: "octocat", + body: "Looks good", + kind: "review", + }, + ]; + + setCachedGitHubStatus(worktreePath, status); + setCachedRepoContext(worktreePath, { + repoUrl: "https://github.com/superset-sh/superset", + upstreamUrl: "https://github.com/superset-sh/superset", + isFork: false, + }); + + const commentsCacheKey = makePullRequestCommentsCacheKey({ + worktreePath, + repoNameWithOwner: "superset-sh/superset", + pullRequestNumber: 2681, + }); + const otherCommentsCacheKey = makePullRequestCommentsCacheKey({ + worktreePath: otherWorktreePath, + repoNameWithOwner: "superset-sh/superset", + pullRequestNumber: 2682, + }); + + setCachedPullRequestComments(commentsCacheKey, comments); + setCachedPullRequestComments(otherCommentsCacheKey, comments); + + clearGitHubCachesForWorktree(worktreePath); + + expect(getCachedGitHubStatus(worktreePath)).toBeNull(); + expect(getCachedRepoContext(worktreePath)).toBeNull(); + expect(getCachedPullRequestComments(commentsCacheKey)).toBeNull(); + expect(getCachedPullRequestComments(otherCommentsCacheKey)).toEqual( + comments, + ); + + clearGitHubCachesForWorktree(otherWorktreePath); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts new file mode 100644 index 00000000000..827770b9be6 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts @@ -0,0 +1,210 @@ +import type { GitHubStatus, PullRequestComment } from "@superset/local-db"; +import type { RepoContext } from "./types"; + +const GITHUB_STATUS_CACHE_TTL_MS = 10_000; +const GITHUB_PR_COMMENTS_CACHE_TTL_MS = 30_000; +const GITHUB_REPO_CONTEXT_CACHE_TTL_MS = 300_000; + +const MAX_GITHUB_STATUS_CACHE_ENTRIES = 256; +const MAX_GITHUB_PR_COMMENTS_CACHE_ENTRIES = 512; +const MAX_GITHUB_REPO_CONTEXT_CACHE_ENTRIES = 256; + +interface CacheEntry { + value: T; + expiresAt: number; +} + +const githubStatusCache = new Map>(); +const githubStatusInFlight = new Map>(); + +const pullRequestCommentsCache = new Map< + string, + CacheEntry +>(); +const pullRequestCommentsInFlight = new Map< + string, + Promise +>(); + +const repoContextCache = new Map>(); +const repoContextInFlight = new Map>(); + +function getCachedValue( + cache: Map>, + cacheKey: string, +): T | null { + const cached = cache.get(cacheKey); + if (!cached) { + return null; + } + + if (cached.expiresAt <= Date.now()) { + cache.delete(cacheKey); + return null; + } + + return cached.value; +} + +function setCachedValue( + cache: Map>, + cacheKey: string, + value: T, + ttlMs: number, + maxEntries: number, +): void { + if (cache.size >= maxEntries) { + cache.clear(); + } + + cache.set(cacheKey, { + value, + expiresAt: Date.now() + ttlMs, + }); +} + +function clearEntriesWithPrefix( + cache: Map, + cacheKeyPrefix: string, +): void { + for (const cacheKey of cache.keys()) { + if (cacheKey.startsWith(cacheKeyPrefix)) { + cache.delete(cacheKey); + } + } +} + +export function getCachedGitHubStatus( + worktreePath: string, +): GitHubStatus | null { + return getCachedValue(githubStatusCache, worktreePath); +} + +export function setCachedGitHubStatus( + worktreePath: string, + value: GitHubStatus, +): void { + setCachedValue( + githubStatusCache, + worktreePath, + value, + GITHUB_STATUS_CACHE_TTL_MS, + MAX_GITHUB_STATUS_CACHE_ENTRIES, + ); +} + +export function getInFlightGitHubStatus( + worktreePath: string, +): Promise | null { + return githubStatusInFlight.get(worktreePath) ?? null; +} + +export function setInFlightGitHubStatus( + worktreePath: string, + promise: Promise, +): void { + githubStatusInFlight.set(worktreePath, promise); +} + +export function clearInFlightGitHubStatus(worktreePath: string): void { + githubStatusInFlight.delete(worktreePath); +} + +export function makePullRequestCommentsCachePrefix( + worktreePath: string, +): string { + return `${worktreePath}::comments::`; +} + +export function makePullRequestCommentsCacheKey({ + worktreePath, + repoNameWithOwner, + pullRequestNumber, +}: { + worktreePath: string; + repoNameWithOwner: string; + pullRequestNumber: number; +}): string { + return `${makePullRequestCommentsCachePrefix(worktreePath)}${repoNameWithOwner}#${pullRequestNumber}`; +} + +export function getCachedPullRequestComments( + cacheKey: string, +): PullRequestComment[] | null { + return getCachedValue(pullRequestCommentsCache, cacheKey); +} + +export function setCachedPullRequestComments( + cacheKey: string, + value: PullRequestComment[], +): void { + setCachedValue( + pullRequestCommentsCache, + cacheKey, + value, + GITHUB_PR_COMMENTS_CACHE_TTL_MS, + MAX_GITHUB_PR_COMMENTS_CACHE_ENTRIES, + ); +} + +export function getInFlightPullRequestComments( + cacheKey: string, +): Promise | null { + return pullRequestCommentsInFlight.get(cacheKey) ?? null; +} + +export function setInFlightPullRequestComments( + cacheKey: string, + promise: Promise, +): void { + pullRequestCommentsInFlight.set(cacheKey, promise); +} + +export function clearInFlightPullRequestComments(cacheKey: string): void { + pullRequestCommentsInFlight.delete(cacheKey); +} + +export function getCachedRepoContext(worktreePath: string): RepoContext | null { + return getCachedValue(repoContextCache, worktreePath); +} + +export function setCachedRepoContext( + worktreePath: string, + value: RepoContext, +): void { + setCachedValue( + repoContextCache, + worktreePath, + value, + GITHUB_REPO_CONTEXT_CACHE_TTL_MS, + MAX_GITHUB_REPO_CONTEXT_CACHE_ENTRIES, + ); +} + +export function getInFlightRepoContext( + worktreePath: string, +): Promise | null { + return repoContextInFlight.get(worktreePath) ?? null; +} + +export function setInFlightRepoContext( + worktreePath: string, + promise: Promise, +): void { + repoContextInFlight.set(worktreePath, promise); +} + +export function clearInFlightRepoContext(worktreePath: string): void { + repoContextInFlight.delete(worktreePath); +} + +export function clearGitHubCachesForWorktree(worktreePath: string): void { + githubStatusCache.delete(worktreePath); + githubStatusInFlight.delete(worktreePath); + repoContextCache.delete(worktreePath); + repoContextInFlight.delete(worktreePath); + + const commentsCachePrefix = makePullRequestCommentsCachePrefix(worktreePath); + clearEntriesWithPrefix(pullRequestCommentsCache, commentsCachePrefix); + clearEntriesWithPrefix(pullRequestCommentsInFlight, commentsCachePrefix); +} diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts new file mode 100644 index 00000000000..a54fadd7a60 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts @@ -0,0 +1,182 @@ +import type { PullRequestComment } from "@superset/local-db"; +import { execWithShellEnv } from "../shell-env"; +import { GHIssueCommentSchema, GHReviewCommentSchema } from "./types"; + +function parseTimestamp(value?: string): number | undefined { + if (!value) { + return undefined; + } + + const timestamp = new Date(value).getTime(); + return Number.isNaN(timestamp) ? undefined : timestamp; +} + +export function parsePaginatedApiArray(stdout: string): unknown[] { + const trimmed = stdout.trim(); + if (!trimmed || trimmed === "null") { + return []; + } + + try { + const raw = JSON.parse(trimmed); + if (!Array.isArray(raw)) { + return []; + } + + return raw.flatMap((page) => (Array.isArray(page) ? page : [page])); + } catch (error) { + console.warn( + "[GitHub] Failed to parse paginated API array response:", + error instanceof Error ? error.message : String(error), + ); + return []; + } +} + +export function parseReviewCommentsResponse( + raw: unknown[], +): PullRequestComment[] { + return raw + .flatMap((item) => { + const result = GHReviewCommentSchema.safeParse(item); + if (!result.success) { + return []; + } + + const comment = result.data; + const body = comment.body?.trim(); + if (!body) { + return []; + } + + return [ + { + id: `review-${comment.id}`, + authorLogin: comment.user?.login || "github", + ...(comment.user?.avatar_url + ? { avatarUrl: comment.user.avatar_url } + : {}), + body, + createdAt: parseTimestamp(comment.created_at), + url: comment.html_url, + kind: "review" as const, + path: comment.path, + line: comment.line ?? comment.original_line ?? undefined, + }, + ]; + }) + .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); +} + +export function parseConversationCommentsResponse( + raw: unknown[], +): PullRequestComment[] { + return raw + .flatMap((item) => { + const result = GHIssueCommentSchema.safeParse(item); + if (!result.success) { + return []; + } + + const comment = result.data; + const body = comment.body?.trim(); + if (!body) { + return []; + } + + return [ + { + id: `conversation-${comment.id}`, + authorLogin: comment.user?.login || "github", + ...(comment.user?.avatar_url + ? { avatarUrl: comment.user.avatar_url } + : {}), + body, + createdAt: parseTimestamp(comment.created_at), + url: comment.html_url, + kind: "conversation" as const, + }, + ]; + }) + .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); +} + +export function mergePullRequestComments( + ...commentGroups: PullRequestComment[][] +): PullRequestComment[] { + const commentsById = new Map(); + + for (const group of commentGroups) { + for (const comment of group) { + commentsById.set(comment.id, comment); + } + } + + return [...commentsById.values()].sort( + (a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0), + ); +} + +async function fetchPaginatedCommentsEndpoint( + worktreePath: string, + endpoint: string, +): Promise { + const { stdout } = await execWithShellEnv( + "gh", + ["api", "--paginate", "--slurp", endpoint], + { cwd: worktreePath }, + ); + + return parsePaginatedApiArray(stdout); +} + +async function fetchReviewCommentsForPullRequest( + worktreePath: string, + repoNameWithOwner: string, + pullRequestNumber: number, +): Promise { + const pages = await fetchPaginatedCommentsEndpoint( + worktreePath, + `repos/${repoNameWithOwner}/pulls/${pullRequestNumber}/comments?per_page=100`, + ); + + return parseReviewCommentsResponse(pages); +} + +async function fetchConversationCommentsForPullRequest( + worktreePath: string, + repoNameWithOwner: string, + pullRequestNumber: number, +): Promise { + const pages = await fetchPaginatedCommentsEndpoint( + worktreePath, + `repos/${repoNameWithOwner}/issues/${pullRequestNumber}/comments?per_page=100`, + ); + + return parseConversationCommentsResponse(pages); +} + +export async function fetchPullRequestComments({ + worktreePath, + repoNameWithOwner, + pullRequestNumber, +}: { + worktreePath: string; + repoNameWithOwner: string; + pullRequestNumber: number; +}): Promise { + const [reviewComments, conversationComments] = await Promise.all([ + fetchReviewCommentsForPullRequest( + worktreePath, + repoNameWithOwner, + pullRequestNumber, + ), + fetchConversationCommentsForPullRequest( + worktreePath, + repoNameWithOwner, + pullRequestNumber, + ), + ]); + + return mergePullRequestComments(reviewComments, conversationComments); +} diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts index 9aad9d6ce57..8a198574a1e 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts @@ -4,6 +4,7 @@ import { getPullRequestRepoArgs, mergePullRequestComments, parseConversationCommentsResponse, + parsePaginatedApiArray, parseReviewCommentsResponse, } from "./github"; @@ -98,6 +99,22 @@ describe("parseReviewCommentsResponse", () => { }); }); +describe("parsePaginatedApiArray", () => { + test("flattens slurped paginated arrays", () => { + expect( + parsePaginatedApiArray( + JSON.stringify([[{ id: 1 }, { id: 2 }], [{ id: 3 }]]), + ), + ).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); + }); + + test("keeps single-page arrays intact", () => { + expect( + parsePaginatedApiArray(JSON.stringify([{ id: 1 }, { id: 2 }])), + ).toEqual([{ id: 1 }, { id: 2 }]); + }); +}); + describe("parseConversationCommentsResponse", () => { test("normalizes top-level PR conversation comments", () => { expect( diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index 6b1b2f145bc..ec808abbc5b 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -6,30 +6,59 @@ import type { import { branchExistsOnRemote, getTrackingRemoteNameForWorktree } from "../git"; import { execGitWithShellPath } from "../git-client"; import { execWithShellEnv } from "../shell-env"; +import { + clearGitHubCachesForWorktree, + clearInFlightGitHubStatus, + clearInFlightPullRequestComments, + clearInFlightRepoContext, + getCachedGitHubStatus, + getCachedPullRequestComments, + getCachedRepoContext, + getInFlightGitHubStatus, + getInFlightPullRequestComments, + getInFlightRepoContext, + makePullRequestCommentsCacheKey, + setCachedGitHubStatus, + setCachedPullRequestComments, + setCachedRepoContext, + setInFlightGitHubStatus, + setInFlightPullRequestComments, + setInFlightRepoContext, +} from "./cache"; +import { fetchPullRequestComments } from "./comments"; import { GHDeploymentSchema, GHDeploymentStatusSchema, - GHIssueCommentSchema, type GHPRResponse, GHPRResponseSchema, GHRepoResponseSchema, - GHReviewCommentSchema, type RepoContext, } from "./types"; -const cache = new Map(); -const CACHE_TTL_MS = 10_000; -const commentsCache = new Map< - string, - { data: PullRequestComment[]; timestamp: number } ->(); -const COMMENTS_CACHE_TTL_MS = 30_000; - -export function clearGitHubStatusCacheForWorktree(worktreePath: string): void { - cache.delete(worktreePath); - commentsCache.delete(worktreePath); +export { + mergePullRequestComments, + parseConversationCommentsResponse, + parsePaginatedApiArray, + parseReviewCommentsResponse, +} from "./comments"; + +export interface PullRequestCommentsTarget { + prNumber: number; + repoContext: Pick; +} + +function getPullRequestCommentsRepoNameWithOwner( + target: PullRequestCommentsTarget, +): string | null { + const targetUrl = target.repoContext.isFork + ? target.repoContext.upstreamUrl + : target.repoContext.repoUrl; + + return extractNwoFromUrl(targetUrl); } +export { clearGitHubCachesForWorktree }; + /** * Fetches GitHub PR status for a worktree using the `gh` CLI. * Returns null if `gh` is not installed, not authenticated, or on error. @@ -37,193 +66,237 @@ export function clearGitHubStatusCacheForWorktree(worktreePath: string): void { export async function fetchGitHubPRStatus( worktreePath: string, ): Promise { - const cached = cache.get(worktreePath); - if (cached && Date.now() - cached.timestamp < CACHE_TTL_MS) { - return cached.data; + const cached = getCachedGitHubStatus(worktreePath); + if (cached) { + return cached; } - try { - const repoContext = await getRepoContext(worktreePath); - if (!repoContext) { - return null; - } + const inFlight = getInFlightGitHubStatus(worktreePath); + if (inFlight) { + return inFlight; + } - const [{ stdout: branchOutput }, { stdout: shaOutput }, trackingRemote] = - await Promise.all([ - execGitWithShellPath(["rev-parse", "--abbrev-ref", "HEAD"], { - cwd: worktreePath, - }), - execGitWithShellPath(["rev-parse", "HEAD"], { cwd: worktreePath }), - getTrackingRemoteNameForWorktree(worktreePath), - ]); - const branchName = branchOutput.trim(); - const headSha = shaOutput.trim(); - - const [branchCheck, prInfo, previewUrl] = await Promise.all([ - branchExistsOnRemote(worktreePath, branchName, trackingRemote), - getPRForBranch(worktreePath, branchName, repoContext, headSha), - fetchPreviewDeploymentUrl(worktreePath, headSha, branchName, repoContext), - ]); - - // If no preview URL found via SHA/branch, try the PR merge ref - // (GitHub Actions pull_request triggers use refs/pull/N/merge) - let finalPreviewUrl = previewUrl; - if (!finalPreviewUrl && prInfo?.number) { - const targetUrl = repoContext.isFork - ? repoContext.upstreamUrl - : repoContext.repoUrl; - const nwo = extractNwoFromUrl(targetUrl); - if (nwo) { - finalPreviewUrl = await queryDeploymentUrl( + const promise = (async () => { + try { + const repoContext = await getRepoContext(worktreePath); + if (!repoContext) { + return null; + } + + const [{ stdout: branchOutput }, { stdout: shaOutput }, trackingRemote] = + await Promise.all([ + execGitWithShellPath(["rev-parse", "--abbrev-ref", "HEAD"], { + cwd: worktreePath, + }), + execGitWithShellPath(["rev-parse", "HEAD"], { cwd: worktreePath }), + getTrackingRemoteNameForWorktree(worktreePath), + ]); + const branchName = branchOutput.trim(); + const headSha = shaOutput.trim(); + + const [branchCheck, prInfo, previewUrl] = await Promise.all([ + branchExistsOnRemote(worktreePath, branchName, trackingRemote), + getPRForBranch(worktreePath, branchName, repoContext, headSha), + fetchPreviewDeploymentUrl( worktreePath, - nwo, - `ref=${encodeURIComponent(`refs/pull/${prInfo.number}/merge`)}`, - ); + headSha, + branchName, + repoContext, + ), + ]); + + // If no preview URL found via SHA/branch, try the PR merge ref + // (GitHub Actions pull_request triggers use refs/pull/N/merge) + let finalPreviewUrl = previewUrl; + if (!finalPreviewUrl && prInfo?.number) { + const targetUrl = repoContext.isFork + ? repoContext.upstreamUrl + : repoContext.repoUrl; + const nwo = extractNwoFromUrl(targetUrl); + if (nwo) { + finalPreviewUrl = await queryDeploymentUrl( + worktreePath, + nwo, + `ref=${encodeURIComponent(`refs/pull/${prInfo.number}/merge`)}`, + ); + } } - } - const result: GitHubStatus = { - pr: prInfo, - repoUrl: repoContext.repoUrl, - upstreamUrl: repoContext.upstreamUrl, - isFork: repoContext.isFork, - branchExistsOnRemote: branchCheck.status === "exists", - previewUrl: finalPreviewUrl, - lastRefreshed: Date.now(), - }; + const result: GitHubStatus = { + pr: prInfo, + repoUrl: repoContext.repoUrl, + upstreamUrl: repoContext.upstreamUrl, + isFork: repoContext.isFork, + branchExistsOnRemote: branchCheck.status === "exists", + previewUrl: finalPreviewUrl, + lastRefreshed: Date.now(), + }; - cache.set(worktreePath, { data: result, timestamp: Date.now() }); + setCachedGitHubStatus(worktreePath, result); - return result; - } catch { - return null; - } -} + return result; + } catch { + return null; + } finally { + clearInFlightGitHubStatus(worktreePath); + } + })(); -export async function fetchGitHubPRComments( - worktreePath: string, -): Promise { - const cached = commentsCache.get(worktreePath); - if (cached && Date.now() - cached.timestamp < COMMENTS_CACHE_TTL_MS) { - return cached.data; - } + setInFlightGitHubStatus(worktreePath, promise); + return promise; +} +export async function fetchGitHubPRComments({ + worktreePath, + pullRequest, +}: { + worktreePath: string; + pullRequest?: PullRequestCommentsTarget | null; +}): Promise { try { - const repoContext = await getRepoContext(worktreePath); - if (!repoContext) { + let pullRequestTarget = pullRequest ?? null; + if (!pullRequestTarget) { + const repoContext = await getRepoContext(worktreePath); + if (!repoContext) { + return []; + } + + const [{ stdout: branchOutput }, { stdout: shaOutput }] = + await Promise.all([ + execGitWithShellPath(["rev-parse", "--abbrev-ref", "HEAD"], { + cwd: worktreePath, + }), + execGitWithShellPath(["rev-parse", "HEAD"], { + cwd: worktreePath, + }), + ]); + const branchName = branchOutput.trim(); + const headSha = shaOutput.trim(); + const prInfo = await getPRForBranch( + worktreePath, + branchName, + repoContext, + headSha, + ); + if (!prInfo) { + return []; + } + + pullRequestTarget = { + prNumber: prInfo.number, + repoContext, + }; + } + + const repoNameWithOwner = + getPullRequestCommentsRepoNameWithOwner(pullRequestTarget); + if (!repoNameWithOwner) { return []; } - const [{ stdout: branchOutput }, { stdout: shaOutput }] = await Promise.all( - [ - execGitWithShellPath(["rev-parse", "--abbrev-ref", "HEAD"], { - cwd: worktreePath, - }), - execGitWithShellPath(["rev-parse", "HEAD"], { cwd: worktreePath }), - ], - ); - const branchName = branchOutput.trim(); - const headSha = shaOutput.trim(); - const prInfo = await getPRForBranch( + const cacheKey = makePullRequestCommentsCacheKey({ worktreePath, - branchName, - repoContext, - headSha, - ); - if (!prInfo) { - return []; + repoNameWithOwner, + pullRequestNumber: pullRequestTarget.prNumber, + }); + const cached = getCachedPullRequestComments(cacheKey); + if (cached) { + return cached; } - const targetUrl = repoContext.isFork - ? repoContext.upstreamUrl - : repoContext.repoUrl; - const nwo = extractNwoFromUrl(targetUrl); - if (!nwo) { - return []; + const inFlight = getInFlightPullRequestComments(cacheKey); + if (inFlight) { + return inFlight; } - const [reviewComments, conversationComments] = await Promise.all([ - fetchReviewCommentsForPR(worktreePath, nwo, prInfo.number), - fetchConversationCommentsForPR(worktreePath, nwo, prInfo.number), - ]); - const comments = mergePullRequestComments( - reviewComments, - conversationComments, - ); - commentsCache.set(worktreePath, { - data: comments, - timestamp: Date.now(), - }); - return comments; + const promise = (async () => { + try { + const comments = await fetchPullRequestComments({ + worktreePath, + repoNameWithOwner, + pullRequestNumber: pullRequestTarget.prNumber, + }); + setCachedPullRequestComments(cacheKey, comments); + return comments; + } finally { + clearInFlightPullRequestComments(cacheKey); + } + })(); + + setInFlightPullRequestComments(cacheKey, promise); + return promise; } catch { return []; } } -const repoContextCache = new Map< - string, - { data: RepoContext; timestamp: number } ->(); -const REPO_CONTEXT_CACHE_TTL_MS = 300_000; // 5 minutes - export async function getRepoContext( worktreePath: string, ): Promise { - const cached = repoContextCache.get(worktreePath); - if (cached && Date.now() - cached.timestamp < REPO_CONTEXT_CACHE_TTL_MS) { - return cached.data; + const cached = getCachedRepoContext(worktreePath); + if (cached) { + return cached; } - try { - const { stdout } = await execWithShellEnv( - "gh", - ["repo", "view", "--json", "url,isFork,parent"], - { cwd: worktreePath }, - ); - const raw = JSON.parse(stdout); - const result = GHRepoResponseSchema.safeParse(raw); - if (!result.success) { - console.error("[GitHub] Repo schema validation failed:", result.error); - console.error("[GitHub] Raw data:", JSON.stringify(raw, null, 2)); - return null; - } + const inFlight = getInFlightRepoContext(worktreePath); + if (inFlight) { + return inFlight; + } - const data = result.data; - let context: RepoContext; + const promise = (async () => { + try { + const { stdout } = await execWithShellEnv( + "gh", + ["repo", "view", "--json", "url,isFork,parent"], + { cwd: worktreePath }, + ); + const raw = JSON.parse(stdout); + const result = GHRepoResponseSchema.safeParse(raw); + if (!result.success) { + console.error("[GitHub] Repo schema validation failed:", result.error); + console.error("[GitHub] Raw data:", JSON.stringify(raw, null, 2)); + return null; + } - if (data.isFork && data.parent) { - context = { - repoUrl: data.url, - upstreamUrl: data.parent.url, - isFork: true, - }; - } else { - const originUrl = await getOriginUrl(worktreePath); - const ghUrl = normalizeGitHubUrl(data.url); + const data = result.data; + let context: RepoContext; - if (originUrl && ghUrl && originUrl !== ghUrl) { + if (data.isFork && data.parent) { context = { - repoUrl: originUrl, - upstreamUrl: ghUrl, + repoUrl: data.url, + upstreamUrl: data.parent.url, isFork: true, }; } else { - context = { - repoUrl: data.url, - upstreamUrl: data.url, - isFork: false, - }; + const originUrl = await getOriginUrl(worktreePath); + const ghUrl = normalizeGitHubUrl(data.url); + + if (originUrl && ghUrl && originUrl !== ghUrl) { + context = { + repoUrl: originUrl, + upstreamUrl: ghUrl, + isFork: true, + }; + } else { + context = { + repoUrl: data.url, + upstreamUrl: data.url, + isFork: false, + }; + } } + + setCachedRepoContext(worktreePath, context); + return context; + } catch { + return null; + } finally { + clearInFlightRepoContext(worktreePath); } + })(); - repoContextCache.set(worktreePath, { - data: context, - timestamp: Date.now(), - }); - return context; - } catch { - return null; - } + setInFlightRepoContext(worktreePath, promise); + return promise; } async function getOriginUrl(worktreePath: string): Promise { @@ -576,143 +649,6 @@ function parseChecks(rollup: GHPRResponse["statusCheckRollup"]): CheckItem[] { }); } -function parseTimestamp(value?: string): number | undefined { - if (!value) { - return undefined; - } - - const timestamp = new Date(value).getTime(); - return Number.isNaN(timestamp) ? undefined : timestamp; -} - -function parseApiArray(stdout: string): unknown[] { - const trimmed = stdout.trim(); - if (!trimmed || trimmed === "null") { - return []; - } - - try { - const raw = JSON.parse(trimmed); - return Array.isArray(raw) ? raw : []; - } catch (error) { - console.warn( - "[GitHub] Failed to parse API array response:", - error instanceof Error ? error.message : String(error), - ); - return []; - } -} - -async function fetchReviewCommentsForPR( - worktreePath: string, - nwo: string, - prNumber: number, -): Promise { - const { stdout } = await execWithShellEnv( - "gh", - ["api", `repos/${nwo}/pulls/${prNumber}/comments?per_page=100`], - { cwd: worktreePath }, - ); - return parseReviewCommentsResponse(parseApiArray(stdout)); -} - -async function fetchConversationCommentsForPR( - worktreePath: string, - nwo: string, - prNumber: number, -): Promise { - const { stdout } = await execWithShellEnv( - "gh", - ["api", `repos/${nwo}/issues/${prNumber}/comments?per_page=100`], - { cwd: worktreePath }, - ); - return parseConversationCommentsResponse(parseApiArray(stdout)); -} - -export function parseReviewCommentsResponse( - raw: unknown[], -): PullRequestComment[] { - return raw - .flatMap((item) => { - const result = GHReviewCommentSchema.safeParse(item); - if (!result.success) { - return []; - } - - const comment = result.data; - const body = comment.body?.trim(); - if (!body) { - return []; - } - - return [ - { - id: `review-${comment.id}`, - authorLogin: comment.user?.login || "github", - ...(comment.user?.avatar_url - ? { avatarUrl: comment.user.avatar_url } - : {}), - body, - createdAt: parseTimestamp(comment.created_at), - url: comment.html_url, - kind: "review" as const, - path: comment.path, - line: comment.line ?? comment.original_line ?? undefined, - }, - ]; - }) - .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); -} - -export function parseConversationCommentsResponse( - raw: unknown[], -): PullRequestComment[] { - return raw - .flatMap((item) => { - const result = GHIssueCommentSchema.safeParse(item); - if (!result.success) { - return []; - } - - const comment = result.data; - const body = comment.body?.trim(); - if (!body) { - return []; - } - - return [ - { - id: `conversation-${comment.id}`, - authorLogin: comment.user?.login || "github", - ...(comment.user?.avatar_url - ? { avatarUrl: comment.user.avatar_url } - : {}), - body, - createdAt: parseTimestamp(comment.created_at), - url: comment.html_url, - kind: "conversation" as const, - }, - ]; - }) - .sort((a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0)); -} - -export function mergePullRequestComments( - ...commentGroups: PullRequestComment[][] -): PullRequestComment[] { - const commentsById = new Map(); - - for (const group of commentGroups) { - for (const comment of group) { - commentsById.set(comment.id, comment); - } - } - - return [...commentsById.values()].sort( - (a, b) => (b.createdAt ?? 0) - (a.createdAt ?? 0), - ); -} - /** * Low-level helper: query deployments matching the given params and return * the environment_url of the first successful deployment. Status lookups diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts index 4fea9921aaf..ff15df691c6 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.ts @@ -1,5 +1,6 @@ +export type { PullRequestCommentsTarget } from "./github"; export { - clearGitHubStatusCacheForWorktree, + clearGitHubCachesForWorktree, fetchGitHubPRComments, fetchGitHubPRStatus, } from "./github"; diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx index 3783e51a31e..c3966d959c3 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx @@ -42,6 +42,10 @@ interface ChangesViewProps { } const INACTIVE_BRANCH_REFETCH_INTERVAL_MS = 10_000; +const GITHUB_STATUS_STALE_TIME_MS = 10_000; +const GITHUB_STATUS_REFETCH_INTERVAL_MS = 10_000; +const GITHUB_PR_COMMENTS_STALE_TIME_MS = 30_000; +const GITHUB_PR_COMMENTS_REFETCH_INTERVAL_MS = 30_000; interface PendingChangesRefresh { invalidateBranches: boolean; @@ -109,7 +113,8 @@ export function ChangesView({ { workspaceId: workspaceId ?? "" }, { enabled: !!workspaceId && isActive, - refetchInterval: isActive ? 10000 : false, + refetchInterval: isActive ? GITHUB_STATUS_REFETCH_INTERVAL_MS : false, + staleTime: GITHUB_STATUS_STALE_TIME_MS, }, ); @@ -250,6 +255,7 @@ export function ChangesView({ useState(false); const [showDiscardStagedDialog, setShowDiscardStagedDialog] = useState(false); const [activeTab, setActiveTab] = useState("diffs"); + const activePullRequest = githubStatus?.pr ?? null; const refreshTimerRef = useRef | null>(null); const pendingRefreshRef = useRef({ invalidateBranches: false, @@ -260,11 +266,25 @@ export function ChangesView({ isLoading: isGitHubCommentsLoading, refetch: refetchGitHubComments, } = electronTrpc.workspaces.getGitHubPRComments.useQuery( - { workspaceId: workspaceId ?? "" }, { - enabled: !!workspaceId && isActive && !!githubStatus?.pr, - refetchInterval: isActive && githubStatus?.pr ? 30_000 : false, - refetchOnWindowFocus: isActive, + workspaceId: workspaceId ?? "", + ...(activePullRequest + ? { + prNumber: activePullRequest.number, + repoUrl: githubStatus?.repoUrl, + upstreamUrl: githubStatus?.upstreamUrl, + isFork: githubStatus?.isFork, + } + : {}), + }, + { + enabled: !!workspaceId && isActive && !!activePullRequest, + refetchInterval: + isActive && activePullRequest + ? GITHUB_PR_COMMENTS_REFETCH_INTERVAL_MS + : false, + staleTime: GITHUB_PR_COMMENTS_STALE_TIME_MS, + refetchOnWindowFocus: false, }, ); @@ -277,7 +297,7 @@ export function ChangesView({ const handleRefresh = () => { refetch(); refetchGithubStatus(); - if (githubStatus?.pr) { + if (activePullRequest) { refetchGitHubComments(); } }; @@ -542,8 +562,8 @@ export function ChangesView({ ]); const hasStagedChanges = stagedFiles.length > 0; - const hasExistingPR = !!githubStatus?.pr; - const prUrl = githubStatus?.pr?.url; + const hasExistingPR = !!activePullRequest; + const prUrl = activePullRequest?.url; const hasGitHubRepo = !!githubStatus?.repoUrl; const defaultBranch = branchData?.defaultBranch ?? status?.defaultBranch ?? ""; @@ -664,7 +684,7 @@ export function ChangesView({ } const againstMainCount = status.againstBase.length; - const reviewCommentCount = githubStatus?.pr ? githubComments.length : 0; + const reviewCommentCount = activePullRequest ? githubComments.length : 0; return (
@@ -772,7 +792,7 @@ export function ChangesView({ className="mt-0 flex min-h-0 flex-1 flex-col outline-none" > ["checks"][number], - prUrl: string, -): string | undefined { - if (check.url) { - return check.url; - } - - const normalizedName = check.name.trim().toLowerCase(); - if ( - normalizedName.includes("coderabbit") || - normalizedName.includes("code rabbit") - ) { - return prUrl; - } - - return undefined; -} - -function getCommentPreview(body: string): string { - return ( - body - .replace(//g, "\n") - .split(/\r?\n/) - .map((line) => line.trim()) - .find(Boolean) - ?.replace(/^[-*+>]\s*/, "") - ?.replace(/\s+/g, " ") ?? "No preview available" - ); -} - -function getAvatarFallback(authorLogin: string): string { - return authorLogin.slice(0, 2).toUpperCase(); -} - -function formatShortAge(timestamp?: number): string | null { - if (!timestamp || Number.isNaN(timestamp)) { - return null; - } - - const deltaMs = Math.max(0, Date.now() - timestamp); - const deltaSeconds = Math.round(deltaMs / 1000); - - if (deltaSeconds < 60) { - return `${Math.max(1, deltaSeconds)}s`; - } - - const deltaMinutes = Math.round(deltaSeconds / 60); - if (deltaMinutes < 60) { - return `${deltaMinutes}m`; - } - - const deltaHours = Math.round(deltaMinutes / 60); - if (deltaHours < 24) { - return `${deltaHours}h`; - } - - return `${Math.round(deltaHours / 24)}d`; -} - -function getCommentLocation(comment: PullRequestComment): string | null { - if (comment.path) { - return comment.line ? `${comment.path}:${comment.line}` : comment.path; - } - - return comment.kind === "conversation" ? "Conversation" : null; -} - -function getCommentKindLabel(comment: PullRequestComment): string { - return comment.kind === "review" ? "Review" : "Comment"; -} - -function formatCommentForClipboard( - comment: PullRequestComment, - includeMetadata = false, -): string { - const body = comment.body.trim() || "No comment body"; - - if (!includeMetadata) { - return body; - } - - const location = getCommentLocation(comment); - const metadata = [ - comment.authorLogin, - getCommentKindLabel(comment), - location, - ].filter(Boolean); - - return [metadata.join(" • "), body].filter(Boolean).join("\n"); -} - -function formatAllCommentsForClipboard(comments: PullRequestComment[]): string { - return comments - .map((comment) => formatCommentForClipboard(comment, true)) - .join("\n\n---\n\n"); -} - export function ReviewPanel({ pr, comments = [], @@ -200,60 +49,62 @@ export function ReviewPanel({ }: ReviewPanelProps) { const [checksOpen, setChecksOpen] = useState(true); const [commentsOpen, setCommentsOpen] = useState(true); - const [copiedItemKey, setCopiedItemKey] = useState(null); - const copyResetTimerRef = useRef | null>(null); - const copyTextMutation = electronTrpc.external.copyText.useMutation(); + const [copiedActionKey, setCopiedActionKey] = useState(null); + const copiedActionResetTimeoutRef = useRef | null>(null); + const copyToClipboardMutation = electronTrpc.external.copyText.useMutation(); useEffect(() => { return () => { - if (copyResetTimerRef.current) { - clearTimeout(copyResetTimerRef.current); + if (copiedActionResetTimeoutRef.current) { + clearTimeout(copiedActionResetTimeoutRef.current); } }; }, []); - const setCopiedState = (key: string) => { - if (copyResetTimerRef.current) { - clearTimeout(copyResetTimerRef.current); + const markCopiedAction = (actionKey: string) => { + if (copiedActionResetTimeoutRef.current) { + clearTimeout(copiedActionResetTimeoutRef.current); } - setCopiedItemKey(key); - copyResetTimerRef.current = setTimeout(() => { - setCopiedItemKey(null); - copyResetTimerRef.current = null; + setCopiedActionKey(actionKey); + copiedActionResetTimeoutRef.current = setTimeout(() => { + setCopiedActionKey(null); + copiedActionResetTimeoutRef.current = null; }, 1500); }; - const handleCopyText = async ({ + const copyTextToClipboard = async ({ text, - key, + actionKey, errorLabel, }: { text: string; - key: string; + actionKey: string; errorLabel: string; }) => { try { - await copyTextMutation.mutateAsync(text); - setCopiedState(key); + await copyToClipboardMutation.mutateAsync(text); + markCopiedAction(actionKey); } catch (error) { const message = error instanceof Error ? error.message : "Unknown error"; toast.error(`${errorLabel}: ${message}`); } }; - const handleCopyComment = (comment: PullRequestComment) => { - void handleCopyText({ - text: formatCommentForClipboard(comment), - key: `comment-${comment.id}`, + const handleCopySingleComment = (comment: PullRequestComment) => { + void copyTextToClipboard({ + text: buildCommentClipboardText(comment), + actionKey: getCommentCopyActionKey(comment.id), errorLabel: "Failed to copy comment", }); }; - const handleCopyAllComments = () => { - void handleCopyText({ - text: formatAllCommentsForClipboard(comments), - key: "all-comments", + const handleCopyCommentsList = () => { + void copyTextToClipboard({ + text: buildAllCommentsClipboardText(comments), + actionKey: ALL_COMMENTS_COPY_ACTION_KEY, errorLabel: "Failed to copy comments", }); }; @@ -324,6 +175,10 @@ export function ReviewPanel({ const checksStatus = relevantChecks.length > 0 ? pr.checksStatus : "none"; const checksStatusConfig = checkSummaryIconConfig[checksStatus]; const ChecksStatusIcon = checksStatusConfig.icon; + const hasComments = comments.length > 0; + const commentsCountLabel = isCommentsLoading ? "..." : comments.length; + const copyAllCommentsLabel = + copiedActionKey === ALL_COMMENTS_COPY_ACTION_KEY ? "Copied" : "Copy all"; return ( @@ -521,10 +372,11 @@ export function ReviewPanel({
) : ( comments.map((comment) => { - const location = getCommentLocation(comment); const age = formatShortAge(comment.createdAt); - const copyKey = `comment-${comment.id}`; - const isCopied = copiedItemKey === copyKey; + const commentCopyActionKey = getCommentCopyActionKey( + comment.id, + ); + const isCopied = copiedActionKey === commentCopyActionKey; const content = ( <> @@ -535,7 +387,7 @@ export function ReviewPanel({ /> ) : null} - {getAvatarFallback(comment.authorLogin)} + {getCommentAvatarFallback(comment.authorLogin)}
@@ -544,7 +396,7 @@ export function ReviewPanel({ {comment.authorLogin} - {getCommentKindLabel(comment)} + {getCommentKindText(comment)} {age && ( @@ -552,18 +404,10 @@ export function ReviewPanel({ )}
- {location && ( -

- {location} -

- )}

- {getCommentPreview(comment.body)} + {getCommentPreviewText(comment.body)}

- {comment.url ? ( - - ) : null} ); @@ -586,24 +430,37 @@ export function ReviewPanel({ {content}
)} - +
+ {comment.url ? ( + + + + ) : null} + +
); }) diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/utils.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/utils.ts new file mode 100644 index 00000000000..7c512011773 --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/utils.ts @@ -0,0 +1,180 @@ +import type { GitHubStatus, PullRequestComment } from "@superset/local-db"; +import { LuCheck, LuLoaderCircle, LuMinus, LuX } from "react-icons/lu"; + +export type PullRequestCheck = NonNullable< + GitHubStatus["pr"] +>["checks"][number]; + +export const ALL_COMMENTS_COPY_ACTION_KEY = "comments:all"; + +export const reviewDecisionConfig = { + approved: { + label: "Approved", + className: + "border border-emerald-500/20 bg-emerald-500/10 text-emerald-700 dark:text-emerald-300", + }, + changes_requested: { + label: "Changes requested", + className: + "border border-red-500/20 bg-red-500/10 text-red-700 dark:text-red-300", + }, + pending: { + label: "Review pending", + className: + "border border-amber-500/20 bg-amber-500/10 text-amber-700 dark:text-amber-300", + }, +} as const; + +export const checkIconConfig = { + success: { + icon: LuCheck, + className: "text-emerald-600 dark:text-emerald-400", + label: "Passed", + }, + failure: { + icon: LuX, + className: "text-red-600 dark:text-red-400", + label: "Failed", + }, + pending: { + icon: LuLoaderCircle, + className: "text-amber-600 dark:text-amber-400", + label: "Pending", + }, + skipped: { + icon: LuMinus, + className: "text-muted-foreground", + label: "Skipped", + }, + cancelled: { + icon: LuMinus, + className: "text-muted-foreground", + label: "Cancelled", + }, +} as const; + +export const checkSummaryIconConfig = { + success: checkIconConfig.success, + failure: checkIconConfig.failure, + pending: checkIconConfig.pending, + none: { + icon: LuMinus, + className: "text-muted-foreground", + label: "No checks", + }, +} as const; + +export const prStateLabel = { + open: "Open", + draft: "Draft", + merged: "Merged", + closed: "Closed", +} as const; + +export function resolveCheckDestinationUrl( + check: PullRequestCheck, + prUrl: string, +): string | undefined { + if (check.url) { + return check.url; + } + + const normalizedName = check.name.trim().toLowerCase(); + if ( + normalizedName.includes("coderabbit") || + normalizedName.includes("code rabbit") + ) { + return prUrl; + } + + return undefined; +} + +export function getCommentPreviewText(body: string): string { + return ( + body + .replace(//g, "\n") + .split(/\r?\n/) + .map((line) => line.trim()) + .find(Boolean) + ?.replace(/^[-*+>]\s*/, "") + ?.replace(/\s+/g, " ") ?? "No preview available" + ); +} + +export function getCommentAvatarFallback(authorLogin: string): string { + return authorLogin.slice(0, 2).toUpperCase(); +} + +export function formatShortAge(timestamp?: number): string | null { + if (!timestamp || Number.isNaN(timestamp)) { + return null; + } + + const deltaMs = Math.max(0, Date.now() - timestamp); + const deltaSeconds = Math.round(deltaMs / 1000); + + if (deltaSeconds < 60) { + return `${Math.max(1, deltaSeconds)}s`; + } + + const deltaMinutes = Math.round(deltaSeconds / 60); + if (deltaMinutes < 60) { + return `${deltaMinutes}m`; + } + + const deltaHours = Math.round(deltaMinutes / 60); + if (deltaHours < 24) { + return `${deltaHours}h`; + } + + return `${Math.round(deltaHours / 24)}d`; +} + +function getCommentClipboardLocation( + comment: PullRequestComment, +): string | null { + if (comment.path) { + return comment.line ? `${comment.path}:${comment.line}` : comment.path; + } + + return comment.kind === "conversation" ? "Conversation" : null; +} + +export function getCommentKindText(comment: PullRequestComment): string { + return comment.kind === "review" ? "Review" : "Comment"; +} + +export function buildCommentClipboardText( + comment: PullRequestComment, + includeMetadata = false, +): string { + const body = comment.body.trim() || "No comment body"; + + if (!includeMetadata) { + return body; + } + + const location = getCommentClipboardLocation(comment); + const metadata = [ + comment.authorLogin, + getCommentKindText(comment), + location, + ].filter(Boolean); + + return [metadata.join(" • "), body].filter(Boolean).join("\n"); +} + +export function buildAllCommentsClipboardText( + comments: PullRequestComment[], +): string { + return comments + .map((comment) => buildCommentClipboardText(comment, true)) + .join("\n\n---\n\n"); +} + +export function getCommentCopyActionKey( + commentId: PullRequestComment["id"], +): string { + return `comment:${commentId}`; +} From f3539a82daf302bebecdcb276ea1d4662cb43837 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 21 Mar 2026 16:50:42 -0700 Subject: [PATCH 5/5] desktop: harden GitHub review fetches --- .../routers/workspaces/utils/github/cache.ts | 2 +- .../workspaces/utils/github/comments.ts | 21 ++++++++++++++++++- .../routers/workspaces/utils/github/github.ts | 4 ++-- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts index 827770b9be6..a0f2375d14f 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts @@ -53,7 +53,7 @@ function setCachedValue( ttlMs: number, maxEntries: number, ): void { - if (cache.size >= maxEntries) { + if (!cache.has(cacheKey) && cache.size >= maxEntries) { cache.clear(); } diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts index a54fadd7a60..f09241b08a7 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/comments.ts @@ -165,7 +165,7 @@ export async function fetchPullRequestComments({ repoNameWithOwner: string; pullRequestNumber: number; }): Promise { - const [reviewComments, conversationComments] = await Promise.all([ + const [reviewResult, conversationResult] = await Promise.allSettled([ fetchReviewCommentsForPullRequest( worktreePath, repoNameWithOwner, @@ -178,5 +178,24 @@ export async function fetchPullRequestComments({ ), ]); + const reviewComments = + reviewResult.status === "fulfilled" ? reviewResult.value : []; + const conversationComments = + conversationResult.status === "fulfilled" ? conversationResult.value : []; + + if (reviewResult.status === "rejected") { + console.warn( + "[GitHub] Failed to fetch pull request review comments:", + reviewResult.reason, + ); + } + + if (conversationResult.status === "rejected") { + console.warn( + "[GitHub] Failed to fetch pull request conversation comments:", + conversationResult.reason, + ); + } + return mergePullRequestComments(reviewComments, conversationComments); } diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index c46a7a780e1..8be88618a7a 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -225,7 +225,7 @@ export async function fetchGitHubPRComments({ const inFlight = getInFlightPullRequestComments(cacheKey); if (inFlight) { - return inFlight; + return await inFlight; } const promise = (async () => { @@ -243,7 +243,7 @@ export async function fetchGitHubPRComments({ })(); setInFlightPullRequestComments(cacheKey, promise); - return promise; + return await promise; } catch { return []; }