diff --git a/apps/code/src/renderer/features/code-editor/stores/diffViewerStore.ts b/apps/code/src/renderer/features/code-editor/stores/diffViewerStore.ts index 7b962fca24..4a5a49442f 100644 --- a/apps/code/src/renderer/features/code-editor/stores/diffViewerStore.ts +++ b/apps/code/src/renderer/features/code-editor/stores/diffViewerStore.ts @@ -4,7 +4,7 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; export type ViewMode = "split" | "unified"; -export type DiffSource = "local" | "branch"; +export type DiffSource = "local" | "branch" | "pr"; interface DiffViewerStoreState { viewMode: ViewMode; diff --git a/apps/code/src/renderer/features/code-review/components/CloudReviewPage.tsx b/apps/code/src/renderer/features/code-review/components/CloudReviewPage.tsx index 07785b5650..ff29e93deb 100644 --- a/apps/code/src/renderer/features/code-review/components/CloudReviewPage.tsx +++ b/apps/code/src/renderer/features/code-review/components/CloudReviewPage.tsx @@ -6,13 +6,8 @@ import { Flex, Spinner, Text } from "@radix-ui/themes"; import { useReviewNavigationStore } from "@renderer/features/code-review/stores/reviewNavigationStore"; import type { Task } from "@shared/types"; import { useMemo } from "react"; -import { LazyDiff } from "./LazyDiff"; -import { PatchedFileDiff } from "./PatchedFileDiff"; -import { - DeferredDiffPlaceholder, - ReviewShell, - useReviewState, -} from "./ReviewShell"; +import { RemoteDiffList } from "./RemoteDiffList"; +import { ReviewShell, useReviewState } from "./ReviewShell"; interface CloudReviewPageProps { task: Task; @@ -52,8 +47,8 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { getDeferredReason, } = useReviewState(reviewFiles, allPaths); - const toolCallDiffs = useMemo(() => { - if (remoteFiles.length > 0) return null; + const toolCallFallbacks = useMemo(() => { + if (remoteFiles.length > 0) return undefined; const diffs = new Map< string, { oldText: string | null; newText: string | null } @@ -97,48 +92,18 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { onCollapseAll={collapseAll} onUncollapseFile={uncollapseFile} > - {reviewFiles.map((file) => { - const isCollapsed = collapsedFiles.has(file.path); - const deferredReason = getDeferredReason(file.path); - - if (deferredReason) { - return ( -
- toggleFile(file.path)} - onShow={() => revealFile(file.path)} - /> -
- ); - } - - const githubFileUrl = prUrl - ? `${prUrl}/files#diff-${file.path.replaceAll("/", "-")}` - : undefined; - - return ( -
- - toggleFile(file.path)} - commentThreads={showReviewComments ? commentThreads : undefined} - fallback={toolCallDiffs?.get(file.path) ?? null} - externalUrl={githubFileUrl} - /> - -
- ); - })} + ); } diff --git a/apps/code/src/renderer/features/code-review/components/DiffSourceSelector.tsx b/apps/code/src/renderer/features/code-review/components/DiffSourceSelector.tsx index 8ee2f5f3d6..f9335ad4ba 100644 --- a/apps/code/src/renderer/features/code-review/components/DiffSourceSelector.tsx +++ b/apps/code/src/renderer/features/code-review/components/DiffSourceSelector.tsx @@ -1,4 +1,9 @@ -import { CaretDown, GitBranch, HardDrives } from "@phosphor-icons/react"; +import { + CaretDown, + GitBranch, + GitPullRequest, + HardDrives, +} from "@phosphor-icons/react"; import { Button, DropdownMenu, @@ -13,6 +18,7 @@ interface DiffSourceSelectorProps { taskId: string; effectiveSource: ResolvedDiffSource; branchAvailable: boolean; + prSourceAvailable: boolean; defaultBranch: string | null; } @@ -20,15 +26,21 @@ export function DiffSourceSelector({ taskId, effectiveSource, branchAvailable, + prSourceAvailable, defaultBranch, }: DiffSourceSelectorProps) { const setDiffSource = useDiffViewerStore((s) => s.setDiffSource); - if (!branchAvailable || !defaultBranch) return null; + const anyAlternative = branchAvailable || prSourceAvailable; + if (!anyAlternative) return null; - const Icon = effectiveSource === "branch" ? GitBranch : HardDrives; - const branchLabel = `Branch vs. ${defaultBranch}`; - const triggerLabel = effectiveSource === "branch" ? branchLabel : "Local"; + const branchLabel = defaultBranch ? `Branch vs. ${defaultBranch}` : "Branch"; + const { icon: Icon, label: triggerLabel } = (() => { + if (effectiveSource === "pr") return { icon: GitPullRequest, label: "PR" }; + if (effectiveSource === "branch") + return { icon: GitBranch, label: branchLabel }; + return { icon: HardDrives, label: "Local" }; + })(); return ( @@ -56,10 +68,18 @@ export function DiffSourceSelector({ Local changes - setDiffSource(taskId, "branch")}> - - {branchLabel} - + {branchAvailable && defaultBranch && ( + setDiffSource(taskId, "branch")}> + + {branchLabel} + + )} + {prSourceAvailable && ( + setDiffSource(taskId, "pr")}> + + Pull request + + )} ); diff --git a/apps/code/src/renderer/features/code-review/components/RemoteDiffList.tsx b/apps/code/src/renderer/features/code-review/components/RemoteDiffList.tsx new file mode 100644 index 0000000000..8b4c2920b7 --- /dev/null +++ b/apps/code/src/renderer/features/code-review/components/RemoteDiffList.tsx @@ -0,0 +1,75 @@ +import type { ChangedFile } from "@shared/types"; +import type { DiffOptions } from "../types"; +import type { PrCommentThread } from "../utils/prCommentAnnotations"; +import { LazyDiff } from "./LazyDiff"; +import { PatchedFileDiff } from "./PatchedFileDiff"; +import { DeferredDiffPlaceholder, type DeferredReason } from "./ReviewShell"; + +interface RemoteDiffListProps { + files: ChangedFile[]; + taskId: string; + options: DiffOptions; + collapsedFiles: Set; + toggleFile: (path: string) => void; + revealFile: (path: string) => void; + getDeferredReason: (path: string) => DeferredReason | null; + prUrl?: string | null; + commentThreads?: Map; + fallbacks?: Map; +} + +export function RemoteDiffList({ + files, + taskId, + options, + collapsedFiles, + toggleFile, + revealFile, + getDeferredReason, + prUrl, + commentThreads, + fallbacks, +}: RemoteDiffListProps) { + return files.map((file) => { + const isCollapsed = collapsedFiles.has(file.path); + const deferredReason = getDeferredReason(file.path); + + if (deferredReason) { + return ( +
+ toggleFile(file.path)} + onShow={() => revealFile(file.path)} + /> +
+ ); + } + + const githubFileUrl = prUrl + ? `${prUrl}/files#diff-${file.path.replaceAll("/", "-")}` + : undefined; + + return ( +
+ + toggleFile(file.path)} + commentThreads={commentThreads} + fallback={fallbacks?.get(file.path) ?? null} + externalUrl={githubFileUrl} + /> + +
+ ); + }); +} diff --git a/apps/code/src/renderer/features/code-review/components/ReviewPage.tsx b/apps/code/src/renderer/features/code-review/components/ReviewPage.tsx index bbd151d567..f43cd51d7f 100644 --- a/apps/code/src/renderer/features/code-review/components/ReviewPage.tsx +++ b/apps/code/src/renderer/features/code-review/components/ReviewPage.tsx @@ -1,5 +1,10 @@ import { useDiffViewerStore } from "@features/code-editor/stores/diffViewerStore"; -import { useGitQueries } from "@features/git-interaction/hooks/useGitQueries"; +import { + useBranchChangedFiles, + useGitQueries, + usePrChangedFiles, +} from "@features/git-interaction/hooks/useGitQueries"; +import { useLinkedBranchPrUrl } from "@features/git-interaction/hooks/useLinkedBranchPrUrl"; import { makeFileKey } from "@features/git-interaction/utils/fileKey"; import { usePanelLayoutStore } from "@features/panels/store/panelLayoutStore"; import { useCwd } from "@features/sidebar/hooks/useCwd"; @@ -19,7 +24,7 @@ import { } from "../utils/resolveDiffSource"; import { InteractiveFileDiff } from "./InteractiveFileDiff"; import { LazyDiff } from "./LazyDiff"; -import { PatchedFileDiff } from "./PatchedFileDiff"; +import { RemoteDiffList } from "./RemoteDiffList"; import { DeferredDiffPlaceholder, type DeferredReason, @@ -30,6 +35,7 @@ import { } from "./ReviewShell"; const EMPTY_BRANCH_FILES: ChangedFile[] = []; +const EMPTY_PR_FILES: ChangedFile[] = []; interface ReviewPageProps { task: Task; @@ -56,14 +62,17 @@ export function ReviewPage({ task }: ReviewPageProps) { defaultBranch, changedFiles: workspaceFiles, } = useGitQueries(repoPath); + const prUrl = useLinkedBranchPrUrl(taskId); const hasLocalChanges = workspaceFiles.length > 0; const branchSourceAvailable = !!linkedBranch && aheadOfDefault > 0; + const prSourceAvailable = !!prUrl; const effectiveSource = resolveDiffSource({ configured: configuredSource, hasLocalChanges, linkedBranch, aheadOfDefault, + prSourceAvailable, }); const isLocalActive = isReviewOpen && effectiveSource === "local"; @@ -119,6 +128,21 @@ export function ReviewPage({ task }: ReviewPageProps) { isReviewOpen={isReviewOpen} effectiveSource={effectiveSource} branchSourceAvailable={branchSourceAvailable} + prSourceAvailable={prSourceAvailable} + /> + ); + } + + if (effectiveSource === "pr") { + return ( + ); } @@ -149,6 +173,7 @@ export function ReviewPage({ task }: ReviewPageProps) { onRefresh={refetch} effectiveSource={effectiveSource} branchSourceAvailable={branchSourceAvailable} + prSourceAvailable={prSourceAvailable} defaultBranch={defaultBranch} > {hasStagedFiles && stagedParsedFiles.length > 0 && ( @@ -196,6 +221,7 @@ function BranchReviewPage({ isReviewOpen, effectiveSource, branchSourceAvailable, + prSourceAvailable, }: { task: Task; branch: string; @@ -204,93 +230,108 @@ function BranchReviewPage({ isReviewOpen: boolean; effectiveSource: ResolvedDiffSource; branchSourceAvailable: boolean; + prSourceAvailable: boolean; }) { const taskId = task.id; - const trpc = useTRPC(); - const repoSlug = repoInfo ? `${repoInfo.organization}/${repoInfo.repository}` : null; - const { data: files = EMPTY_BRANCH_FILES, isLoading } = useQuery( - trpc.git.getBranchChangedFiles.queryOptions( - { repo: repoSlug as string, branch }, - { - enabled: isReviewOpen && !!repoSlug, - staleTime: 30_000, - refetchInterval: 30_000, - retry: 1, - }, - ), + const { data: files = EMPTY_BRANCH_FILES, isLoading } = useBranchChangedFiles( + isReviewOpen ? repoSlug : null, + isReviewOpen ? branch : null, ); const allPaths = useMemo(() => files.map((f) => f.path), [files]); - const { - diffOptions, - linesAdded, - linesRemoved, - collapsedFiles, - toggleFile, - expandAll, - collapseAll, - uncollapseFile, - revealFile, - getDeferredReason, - } = useReviewState(files, allPaths); + const reviewState = useReviewState(files, allPaths); return ( - {files.map((file) => { - const isCollapsed = collapsedFiles.has(file.path); - const deferredReason = getDeferredReason(file.path); - - if (deferredReason) { - return ( -
- toggleFile(file.path)} - onShow={() => revealFile(file.path)} - /> -
- ); - } + +
+ ); +} - return ( -
- - toggleFile(file.path)} - /> - -
- ); - })} +function PrReviewPage({ + task, + prUrl, + defaultBranch, + isReviewOpen, + effectiveSource, + branchSourceAvailable, + prSourceAvailable, +}: { + task: Task; + prUrl: string; + defaultBranch: string | null; + isReviewOpen: boolean; + effectiveSource: ResolvedDiffSource; + branchSourceAvailable: boolean; + prSourceAvailable: boolean; +}) { + const taskId = task.id; + + const { data: files = EMPTY_PR_FILES, isLoading } = usePrChangedFiles( + isReviewOpen ? prUrl : null, + ); + + const allPaths = useMemo(() => files.map((f) => f.path), [files]); + + const reviewState = useReviewState(files, allPaths); + + return ( + + ); } diff --git a/apps/code/src/renderer/features/code-review/components/ReviewShell.tsx b/apps/code/src/renderer/features/code-review/components/ReviewShell.tsx index 9449375257..7fb1d8c41e 100644 --- a/apps/code/src/renderer/features/code-review/components/ReviewShell.tsx +++ b/apps/code/src/renderer/features/code-review/components/ReviewShell.tsx @@ -298,6 +298,7 @@ export interface ReviewShellProps { onRefresh?: () => void; effectiveSource?: ResolvedDiffSource; branchSourceAvailable?: boolean; + prSourceAvailable?: boolean; defaultBranch?: string | null; } @@ -316,6 +317,7 @@ export function ReviewShell({ onRefresh, effectiveSource, branchSourceAvailable, + prSourceAvailable, defaultBranch, }: ReviewShellProps) { const taskId = task.id; @@ -424,6 +426,7 @@ export function ReviewShell({ onRefresh={onRefresh} effectiveSource={effectiveSource} branchSourceAvailable={branchSourceAvailable} + prSourceAvailable={prSourceAvailable} defaultBranch={defaultBranch} /> diff --git a/apps/code/src/renderer/features/code-review/components/ReviewToolbar.tsx b/apps/code/src/renderer/features/code-review/components/ReviewToolbar.tsx index 06879c7425..8d146aa9b9 100644 --- a/apps/code/src/renderer/features/code-review/components/ReviewToolbar.tsx +++ b/apps/code/src/renderer/features/code-review/components/ReviewToolbar.tsx @@ -24,6 +24,7 @@ interface ReviewToolbarProps { onRefresh?: () => void; effectiveSource?: ResolvedDiffSource; branchSourceAvailable?: boolean; + prSourceAvailable?: boolean; defaultBranch?: string | null; } @@ -36,6 +37,7 @@ export const ReviewToolbar = memo(function ReviewToolbar({ onRefresh, effectiveSource, branchSourceAvailable, + prSourceAvailable, defaultBranch, }: ReviewToolbarProps) { const viewMode = useDiffViewerStore((s) => s.viewMode); @@ -74,6 +76,7 @@ export const ReviewToolbar = memo(function ReviewToolbar({ taskId={taskId} effectiveSource={effectiveSource} branchAvailable={branchSourceAvailable ?? false} + prSourceAvailable={prSourceAvailable ?? false} defaultBranch={defaultBranch ?? null} /> )} diff --git a/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.test.ts b/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.test.ts index 14f92ac88f..5bd96019d9 100644 --- a/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.test.ts +++ b/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.test.ts @@ -9,68 +9,107 @@ describe("resolveDiffSource", () => { it.each< ResolveDiffSourceInput & { expected: ResolvedDiffSource; desc: string } >([ + // Heuristic (no user choice) — dirty > pr > branch > local. { - desc: "heuristic: uncommitted changes → local", + desc: "heuristic: uncommitted changes → local (even with PR)", configured: null, hasLocalChanges: true, linkedBranch: "feat/x", aheadOfDefault: 3, + prSourceAvailable: true, expected: "local", }, { - desc: "heuristic: clean tree with commits ahead → branch", + desc: "heuristic: clean tree with PR → pr", configured: null, hasLocalChanges: false, linkedBranch: "feat/x", aheadOfDefault: 2, + prSourceAvailable: true, + expected: "pr", + }, + { + desc: "heuristic: clean tree with commits ahead (no PR) → branch", + configured: null, + hasLocalChanges: false, + linkedBranch: "feat/x", + aheadOfDefault: 2, + prSourceAvailable: false, expected: "branch", }, { - desc: "heuristic: no linked branch → local", + desc: "heuristic: no linked branch, no PR → local", configured: null, hasLocalChanges: false, linkedBranch: null, aheadOfDefault: 0, + prSourceAvailable: false, expected: "local", }, { - desc: "heuristic: linked branch but no commits ahead → local", + desc: "heuristic: linked branch but no commits ahead, no PR → local", configured: null, hasLocalChanges: false, linkedBranch: "feat/x", aheadOfDefault: 0, + prSourceAvailable: false, expected: "local", }, + // Explicit local. { - desc: "explicit local respected even when branch is available", + desc: "explicit local respected even when PR is available", configured: "local", hasLocalChanges: false, linkedBranch: "feat/x", aheadOfDefault: 5, + prSourceAvailable: true, expected: "local", }, + // Explicit branch. { desc: "explicit branch respected when available", configured: "branch", hasLocalChanges: true, linkedBranch: "feat/x", aheadOfDefault: 1, + prSourceAvailable: false, expected: "branch", }, { - desc: "explicit branch falls back to local when no linked branch", + desc: "explicit branch falls back to local when unavailable", configured: "branch", hasLocalChanges: false, linkedBranch: null, aheadOfDefault: 0, + prSourceAvailable: false, expected: "local", }, + // Explicit pr. { - desc: "explicit branch falls back to local when no commits ahead", - configured: "branch", + desc: "explicit pr respected when available", + configured: "pr", + hasLocalChanges: true, + linkedBranch: "feat/x", + aheadOfDefault: 1, + prSourceAvailable: true, + expected: "pr", + }, + { + desc: "explicit pr falls back to branch when PR unavailable but branch is", + configured: "pr", hasLocalChanges: false, linkedBranch: "feat/x", + aheadOfDefault: 3, + prSourceAvailable: false, + expected: "branch", + }, + { + desc: "explicit pr falls back to local when nothing else available", + configured: "pr", + hasLocalChanges: false, + linkedBranch: null, aheadOfDefault: 0, + prSourceAvailable: false, expected: "local", }, ])("$desc", ({ expected, ...input }) => { diff --git a/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.ts b/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.ts index 307351da3f..a82e80af23 100644 --- a/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.ts +++ b/apps/code/src/renderer/features/code-review/utils/resolveDiffSource.ts @@ -7,6 +7,7 @@ export interface ResolveDiffSourceInput { hasLocalChanges: boolean; linkedBranch: string | null; aheadOfDefault: number; + prSourceAvailable: boolean; } export function resolveDiffSource({ @@ -14,9 +15,15 @@ export function resolveDiffSource({ hasLocalChanges, linkedBranch, aheadOfDefault, + prSourceAvailable, }: ResolveDiffSourceInput): ResolvedDiffSource { const branchAvailable = !!linkedBranch && aheadOfDefault > 0; + if (configured === "pr") { + if (prSourceAvailable) return "pr"; + if (branchAvailable) return "branch"; + return "local"; + } if (configured === "branch") { return branchAvailable ? "branch" : "local"; } @@ -25,6 +32,7 @@ export function resolveDiffSource({ } if (hasLocalChanges) return "local"; + if (prSourceAvailable) return "pr"; if (branchAvailable) return "branch"; return "local"; } diff --git a/apps/code/src/renderer/features/git-interaction/hooks/useGitQueries.ts b/apps/code/src/renderer/features/git-interaction/hooks/useGitQueries.ts index 824e2c352b..dee825939b 100644 --- a/apps/code/src/renderer/features/git-interaction/hooks/useGitQueries.ts +++ b/apps/code/src/renderer/features/git-interaction/hooks/useGitQueries.ts @@ -154,28 +154,25 @@ export function useGitQueries(repoPath?: string) { }; } -export function useCloudPrChangedFiles( - prUrl: string | null, - isRunActive?: boolean, -) { +export function usePrChangedFiles(prUrl: string | null, pollFast?: boolean) { const trpc = useTRPC(); return useQuery( trpc.git.getPrChangedFiles.queryOptions( { prUrl: prUrl as string }, { enabled: !!prUrl, - staleTime: isRunActive ? 10_000 : 5 * 60_000, - refetchInterval: isRunActive ? 10_000 : false, + staleTime: pollFast ? 10_000 : 5 * 60_000, + refetchInterval: pollFast ? 10_000 : false, retry: 1, }, ), ); } -export function useCloudBranchChangedFiles( +export function useBranchChangedFiles( repo: string | null, branch: string | null, - isRunActive?: boolean, + pollFast?: boolean, ) { const trpc = useTRPC(); return useQuery( @@ -183,8 +180,8 @@ export function useCloudBranchChangedFiles( { repo: repo as string, branch: branch as string }, { enabled: !!repo && !!branch, - staleTime: isRunActive ? 10_000 : 30_000, - refetchInterval: isRunActive ? 10_000 : 30_000, + staleTime: pollFast ? 10_000 : 30_000, + refetchInterval: pollFast ? 10_000 : 30_000, retry: 1, }, ), diff --git a/apps/code/src/renderer/features/task-detail/hooks/useCloudChangedFiles.ts b/apps/code/src/renderer/features/task-detail/hooks/useCloudChangedFiles.ts index a02df93f9a..df2f4c32b4 100644 --- a/apps/code/src/renderer/features/task-detail/hooks/useCloudChangedFiles.ts +++ b/apps/code/src/renderer/features/task-detail/hooks/useCloudChangedFiles.ts @@ -1,6 +1,6 @@ import { - useCloudBranchChangedFiles, - useCloudPrChangedFiles, + useBranchChangedFiles, + usePrChangedFiles, } from "@features/git-interaction/hooks/useGitQueries"; import { useCloudRunState } from "@features/task-detail/hooks/useCloudRunState"; import type { ChangedFile, Task } from "@shared/types"; @@ -20,13 +20,13 @@ export function useCloudChangedFiles( data: prFiles, isPending: prPending, isError: prError, - } = useCloudPrChangedFiles(isActive ? prUrl : null, isRunActive); + } = usePrChangedFiles(isActive ? prUrl : null, isRunActive); const { data: branchFiles, isPending: branchPending, isError: branchError, - } = useCloudBranchChangedFiles( + } = useBranchChangedFiles( isActive && !prUrl ? repo : null, isActive && !prUrl ? effectiveBranch : null, isRunActive,