diff --git a/apps/web/components/github/pr-detail-panel.tsx b/apps/web/components/github/pr-detail-panel.tsx index 83fd610876..ac7261c844 100644 --- a/apps/web/components/github/pr-detail-panel.tsx +++ b/apps/web/components/github/pr-detail-panel.tsx @@ -17,7 +17,7 @@ import { ScrollArea } from "@kandev/ui/scroll-area"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import { useAppStore } from "@/components/state-provider"; import { useActiveTaskPR, useTaskPR } from "@/hooks/domains/github/use-task-pr"; -import { prPanelLabel } from "@/components/github/pr-utils"; +import { prPanelLabel, prTaskKey } from "@/components/github/pr-utils"; import { usePRFeedback } from "@/hooks/domains/github/use-pr-feedback"; import { useGitHubStatus } from "@/hooks/domains/github/use-github-status"; import { useCommentsStore, isPRFeedbackComment } from "@/lib/state/slices/comments"; @@ -78,11 +78,6 @@ export function PRDetailPanelComponent({ panelId, params }: PRDetailPanelProps) ); } -/** Stable per-PR key used by addPRPanel and the multi-PR topbar buttons. */ -export function prTaskKey(pr: TaskPR): string { - return `${pr.owner}/${pr.repo}/${pr.pr_number}`; -} - // --- Add PR feedback as chat context --- function useAddPRFeedbackAsContext(sessionId: string, prNumber: number) { diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index 478d47aa5b..426163807d 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -34,8 +34,7 @@ import { isPRReadyToMerge, isPRWaitingOnBranchProtection, } from "@/components/github/pr-task-icon"; -import { prTaskKey } from "@/components/github/pr-detail-panel"; -import { prIdentitySlug } from "@/components/github/pr-utils"; +import { prIdentitySlug, prTaskKey } from "@/components/github/pr-utils"; import { PR_CI_DESKTOP_POPOVER_SCROLL_CLASS, PRCIPopover } from "@/components/github/pr-ci-popover"; import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover"; import { useAppStore } from "@/components/state-provider"; diff --git a/apps/web/components/github/pr-utils.ts b/apps/web/components/github/pr-utils.ts index 7b11411e92..cc78fa2ac1 100644 --- a/apps/web/components/github/pr-utils.ts +++ b/apps/web/components/github/pr-utils.ts @@ -13,3 +13,8 @@ export function prPanelLabel(prNumber: number): string { export function prIdentitySlug(pr: TaskPR): string { return `${pr.owner}-${pr.repo}-${pr.pr_number}`; } + +/** Stable per-PR key used by task-scoped state and dockview panels. */ +export function prTaskKey(pr: TaskPR): string { + return `${pr.owner}/${pr.repo}/${pr.pr_number}`; +} diff --git a/apps/web/components/review/review-dialog-pr-state.test.tsx b/apps/web/components/review/review-dialog-pr-state.test.tsx new file mode 100644 index 0000000000..30c91c544f --- /dev/null +++ b/apps/web/components/review/review-dialog-pr-state.test.tsx @@ -0,0 +1,98 @@ +import { act, cleanup, fireEvent, render, renderHook, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { TaskPR } from "@/lib/types/github"; +import { + ReviewPRDiffBoundary, + shouldBlockReviewForPR, + useReviewDialogTransientState, +} from "./review-dialog-pr-state"; + +afterEach(cleanup); + +const selectedPR = { + repo: "widgets", + pr_number: 42, +} as TaskPR; + +describe("ReviewPRDiffBoundary", () => { + it("keeps expanded Review usable when local files exist during a PR failure", () => { + expect( + shouldBlockReviewForPR([ + { + path: "src/local.ts", + diff: "@@ -1 +1 @@", + status: "modified", + additions: 1, + deletions: 1, + staged: false, + source: "uncommitted", + }, + ]), + ).toBe(false); + }); + + it("blocks expanded Review when only PR files are visible", () => { + expect( + shouldBlockReviewForPR([ + { + path: "src/pr.ts", + diff: "@@ -1 +1 @@", + status: "modified", + additions: 1, + deletions: 1, + staged: false, + source: "pr", + }, + ]), + ).toBe(true); + }); + + it("retries a failed selected PR without rendering stale children", () => { + const onRetry = vi.fn(); + render( + + stale diff + , + ); + + expect(screen.queryByText("stale diff")).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(onRetry).toHaveBeenCalledOnce(); + }); + + it("ignores PR fetch state for a local-only review source", () => { + render( + + local diff + , + ); + + expect(screen.getByText("local diff")).toBeTruthy(); + }); +}); + +describe("useReviewDialogTransientState", () => { + it("derives cleared state for a new source and keeps subsequent edits on that source", () => { + const { result, rerender } = renderHook( + ({ sourceKey }) => useReviewDialogTransientState(sourceKey), + { initialProps: { sourceKey: "session-a:pr-1" } }, + ); + + act(() => { + result.current.setSelectedFile("src/old.ts"); + result.current.setFilter("old"); + }); + rerender({ sourceKey: "session-b:pr-1" }); + + expect(result.current.selectedFile).toBeNull(); + expect(result.current.filter).toBe(""); + + act(() => result.current.setFilter("new")); + expect(result.current.filter).toBe("new"); + }); +}); diff --git a/apps/web/components/review/review-dialog-pr-state.tsx b/apps/web/components/review/review-dialog-pr-state.tsx new file mode 100644 index 0000000000..20a197b282 --- /dev/null +++ b/apps/web/components/review/review-dialog-pr-state.tsx @@ -0,0 +1,156 @@ +"use client"; + +import { useCallback, useEffect, useRef, useState, type ReactNode } from "react"; +import { IconLoader2, IconRefresh } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import type { TaskPR } from "@/lib/types/github"; +import type { ReviewFile } from "./types"; + +type AutoCloseReviewDialogInput = { + open: boolean; + previousFileCount: number | null; + fileCount: number; + prDiffLoading: boolean; +}; + +export function shouldAutoCloseReviewDialog({ + open, + previousFileCount, + fileCount, + prDiffLoading, +}: AutoCloseReviewDialogInput): boolean { + return ( + open && !prDiffLoading && previousFileCount !== null && previousFileCount > 0 && fileCount === 0 + ); +} + +export function useReviewDialogAutoClose(opts: { + open: boolean; + fileCount: number; + prDiffLoading: boolean; + onOpenChange: (open: boolean) => void; +}) { + const previousFileCountRef = useRef(null); + useEffect(() => { + if ( + shouldAutoCloseReviewDialog({ + open: opts.open, + previousFileCount: previousFileCountRef.current, + fileCount: opts.fileCount, + prDiffLoading: opts.prDiffLoading, + }) + ) { + opts.onOpenChange(false); + } + previousFileCountRef.current = opts.fileCount; + }, [opts.open, opts.fileCount, opts.prDiffLoading, opts.onOpenChange]); +} + +export type ReviewTransientState = { + sourceKey: string; + selectedFile: string | null; + filter: string; +}; + +export function reviewDialogSourceKey(sessionId: string, selectedPRKey: string | null): string { + return `${sessionId}\u0000${selectedPRKey ?? "review-without-pr"}`; +} + +export function shouldBlockReviewForPR(files: ReviewFile[]): boolean { + return !files.some((file) => file.source !== "pr"); +} + +export function resolveReviewTransientState( + state: ReviewTransientState, + sourceKey: string, +): ReviewTransientState { + if (state.sourceKey === sourceKey) return state; + return { sourceKey, selectedFile: null, filter: "" }; +} + +export function useReviewDialogTransientState(sourceKey: string) { + const [state, setState] = useState(() => ({ + sourceKey, + selectedFile: null, + filter: "", + })); + const resolvedState = resolveReviewTransientState(state, sourceKey); + useEffect(() => { + setState((current) => resolveReviewTransientState(current, sourceKey)); + }, [sourceKey]); + + const setSelectedFile = useCallback( + (value: string | null) => + setState((current) => ({ + ...resolveReviewTransientState(current, sourceKey), + selectedFile: value, + })), + [sourceKey], + ); + const setFilter = useCallback( + (value: string) => + setState((current) => ({ + ...resolveReviewTransientState(current, sourceKey), + filter: value, + })), + [sourceKey], + ); + + return { + selectedFile: resolvedState.selectedFile, + filter: resolvedState.filter, + setSelectedFile, + setFilter, + }; +} + +export function usePRKeyedReviewFileSelection( + selectFile: (path: string, setSelectedFile: (value: string | null) => void) => void, + setSelectedFile: (value: string | null) => void, +) { + return useCallback( + (path: string) => selectFile(path, setSelectedFile), + [selectFile, setSelectedFile], + ); +} + +type ReviewPRDiffBoundaryProps = { + selectedPR: TaskPR | null; + loading: boolean; + error: string | null; + onRetry?: () => void; + children: ReactNode; +}; + +export function ReviewPRDiffBoundary({ + selectedPR, + loading, + error, + onRetry, + children, +}: ReviewPRDiffBoundaryProps) { + if (selectedPR && loading) { + return ( +
+ + + Loading {selectedPR.repo} #{selectedPR.pr_number} changes… + +
+ ); + } + if (selectedPR && error) { + return ( +
+ {error} + {onRetry && ( + + )} +
+ ); + } + return children; +} diff --git a/apps/web/components/review/review-dialog-surface.tsx b/apps/web/components/review/review-dialog-surface.tsx new file mode 100644 index 0000000000..0eec210cdf --- /dev/null +++ b/apps/web/components/review/review-dialog-surface.tsx @@ -0,0 +1,147 @@ +"use client"; + +import { useRef } from "react"; +import { Dialog, DialogContent, DialogTitle } from "@kandev/ui/dialog"; +import { useReviewSidebarResize } from "@/hooks/use-review-sidebar-resize"; +import type { TaskPR } from "@/lib/types/github"; +import type { ReviewDialogViewState } from "./review-dialog"; +import { ReviewDiffList } from "./review-diff-list"; +import { ReviewFileTree } from "./review-file-tree"; +import { ReviewPRDiffBoundary, shouldBlockReviewForPR } from "./review-dialog-pr-state"; +import { ReviewTopBar } from "./review-top-bar"; + +type ReviewDialogSurfaceProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + sessionId: string; + baseBranch?: string; + onOpenFile?: (filePath: string) => void; + prs: TaskPR[]; + selectedPR: TaskPR | null; + onSelectPR?: (pr: TaskPR) => void; + prDiffLoading: boolean; + prDiffError: string | null; + onRetryPRDiff?: () => void; + onRequestWalkthrough: () => void; + state: ReviewDialogViewState; +}; + +function ReviewDialogDiffContent({ + sessionId, + onOpenFile, + selectedPR, + prDiffLoading, + prDiffError, + onRetryPRDiff, + state, +}: Pick< + ReviewDialogSurfaceProps, + | "sessionId" + | "onOpenFile" + | "selectedPR" + | "prDiffLoading" + | "prDiffError" + | "onRetryPRDiff" + | "state" +>) { + const blockReviewForPR = shouldBlockReviewForPR(state.allFiles); + return ( + + {state.filteredFiles.length > 0 ? ( + + ) : ( +
+ {state.filter.trim() ? "No files match the filter" : "No changes to review"} +
+ )} + + ); +} + +export function ReviewDialogSurface(props: ReviewDialogSurfaceProps) { + const { open, onOpenChange, sessionId, state } = props; + const splitRowRef = useRef(null); + const sidebar = useReviewSidebarResize(splitRowRef, open, state.reviewSourceKey); + + return ( + + + Review Changes + onOpenChange(false)} + onRequestWalkthrough={props.onRequestWalkthrough} + requestWalkthroughDisabled={state.allFiles.length === 0} + getPendingComments={state.getPendingComments} + markCommentsSent={state.markCommentsSent} + prs={props.prs} + selectedPR={props.selectedPR} + onSelectPR={props.onSelectPR} + prDiffLoading={props.prDiffLoading} + /> +
+
+ +
+ +
+ +
+
+ + + ); +} diff --git a/apps/web/components/review/review-dialog.build-files.test.ts b/apps/web/components/review/review-dialog.build-files.test.ts index a6d33ca7b4..0b54cb06bc 100644 --- a/apps/web/components/review/review-dialog.build-files.test.ts +++ b/apps/web/components/review/review-dialog.build-files.test.ts @@ -1,5 +1,14 @@ import { describe, it, expect } from "vitest"; +import { act, renderHook } from "@testing-library/react"; import { buildAllFiles, filterPendingDiffCommentsForSession } from "./review-dialog"; +import { + reviewDialogSourceKey, + resolveReviewTransientState, + shouldAutoCloseReviewDialog, + usePRKeyedReviewFileSelection, + useReviewDialogTransientState, + type ReviewTransientState, +} from "./review-dialog-pr-state"; import { reviewFileKey } from "./types"; import type { CumulativeDiff } from "@/lib/state/slices/session-runtime/types"; import type { Comment } from "@/lib/state/slices/comments"; @@ -9,6 +18,88 @@ const PREVIOUS_PATH = "src/old-name.ts"; const README_PATH = "README.md"; const FRONTEND_REPO = "frontend"; const BACKEND_REPO = "backend"; +const FIRST_PR_KEY = "acme/app/1"; +const SECOND_PR_KEY = "acme/app/2"; + +describe("Review dialog PR transitions", () => { + it("keeps Review open while the selected PR diff is loading", () => { + expect( + shouldAutoCloseReviewDialog({ + open: true, + previousFileCount: 2, + fileCount: 0, + prDiffLoading: true, + }), + ).toBe(false); + }); + + it("resets file and filter state when the selected PR changes", () => { + const current: ReviewTransientState = { + sourceKey: FIRST_PR_KEY, + selectedFile: "src/first-pr.ts", + filter: "first-pr", + }; + expect(resolveReviewTransientState(current, SECOND_PR_KEY)).toEqual({ + sourceKey: SECOND_PR_KEY, + selectedFile: null, + filter: "", + }); + }); + + it("does not restore stale file and filter state after switching away and back", () => { + const { result, rerender } = renderHook( + ({ sourceKey }: { sourceKey: string }) => useReviewDialogTransientState(sourceKey), + { initialProps: { sourceKey: FIRST_PR_KEY } }, + ); + + act(() => { + result.current.setSelectedFile("src/first-pr.ts"); + result.current.setFilter("first-pr"); + }); + rerender({ sourceKey: SECOND_PR_KEY }); + rerender({ sourceKey: FIRST_PR_KEY }); + + expect(result.current.selectedFile).toBeNull(); + expect(result.current.filter).toBe(""); + }); + + it("resets file and filter state when the session changes under the same PR", () => { + const { result, rerender } = renderHook( + ({ sessionId }) => + useReviewDialogTransientState(reviewDialogSourceKey(sessionId, FIRST_PR_KEY)), + { initialProps: { sessionId: "session-1" } }, + ); + + act(() => { + result.current.setSelectedFile("src/first-pr.ts"); + result.current.setFilter("first-pr"); + }); + rerender({ sessionId: "session-2" }); + + expect(result.current.selectedFile).toBeNull(); + expect(result.current.filter).toBe(""); + }); + + it("selects files with the current PR setter after switching PRs", () => { + const selectFile = (path: string, setSelectedFile: (value: string | null) => void) => + setSelectedFile(path); + const { result, rerender } = renderHook( + ({ sourceKey }: { sourceKey: string }) => { + const transient = useReviewDialogTransientState(sourceKey); + return { + ...transient, + handleSelectFile: usePRKeyedReviewFileSelection(selectFile, transient.setSelectedFile), + }; + }, + { initialProps: { sourceKey: FIRST_PR_KEY } }, + ); + + rerender({ sourceKey: SECOND_PR_KEY }); + act(() => result.current.handleSelectFile("src/second-pr.ts")); + + expect(result.current.selectedFile).toBe("src/second-pr.ts"); + }); +}); function pendingDiffComment(overrides: Partial): Comment { return { diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index d9429bc2a7..84f44539fd 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -1,24 +1,26 @@ "use client"; -import { memo, useMemo, useCallback, createRef, useState, useRef, useEffect } from "react"; -import { Dialog, DialogContent, DialogTitle } from "@kandev/ui/dialog"; +import { memo, useMemo, useCallback, createRef, useState, useEffect } from "react"; import type { DiffComment } from "@/lib/diff/types"; import type { FileInfo, CumulativeDiff } from "@/lib/state/slices/session-runtime/types"; -import type { PRDiffFile } from "@/lib/types/github"; +import type { PRDiffFile, TaskPR } from "@/lib/types/github"; import type { Comment } from "@/lib/state/slices/comments"; import { useCommentsStore, isDiffComment } from "@/lib/state/slices/comments"; import { useSessionFileReviews } from "@/hooks/use-session-file-reviews"; import { useGitOperations } from "@/hooks/use-git-operations"; import { walkthroughStepMatchesFile } from "@/lib/diff/walkthrough-match"; -import { useReviewSidebarResize } from "@/hooks/use-review-sidebar-resize"; import { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; import { DEFAULT_DIFF_WORD_WRAP } from "@/components/diff/diff-defaults"; import { normalizeFileChangeStatus } from "@/lib/utils/file-change-status"; import { useRequestChangesWalkthrough } from "@/hooks/domains/session/use-request-changes-walkthrough"; -import { ReviewTopBar } from "./review-top-bar"; -import { ReviewFileTree } from "./review-file-tree"; -import { ReviewDiffList } from "./review-diff-list"; +import { ReviewDialogSurface } from "./review-dialog-surface"; +import { + reviewDialogSourceKey, + usePRKeyedReviewFileSelection, + useReviewDialogAutoClose, + useReviewDialogTransientState, +} from "./review-dialog-pr-state"; import type { ReviewFile } from "./types"; import { hashDiff, @@ -162,7 +164,7 @@ export function filterPendingDiffCommentsForSession( ); } -type ReviewDialogProps = { +export type ReviewDialogProps = { open: boolean; onOpenChange: (open: boolean) => void; sessionId: string; @@ -171,7 +173,14 @@ type ReviewDialogProps = { onOpenFile?: (filePath: string) => void; gitStatusFiles: Record | null; cumulativeDiff: CumulativeDiff | null; + prs?: TaskPR[]; + selectedPR?: TaskPR | null; + selectedPRKey?: string | null; + onSelectPR?: (pr: TaskPR) => void; prDiffFiles?: PRDiffFile[]; + prDiffLoading?: boolean; + prDiffError?: string | null; + onRetryPRDiff?: () => void; prRepoName?: string; useRepositoryKeys?: boolean; }; @@ -343,11 +352,15 @@ function useReviewDialogState(props: ReviewDialogProps) { onSendComments, gitStatusFiles, cumulativeDiff, + selectedPRKey = null, prDiffFiles, + prDiffLoading = false, prRepoName, useRepositoryKeys = true, } = props; - const [selectedFile, setSelectedFile] = useState(null); + const reviewSourceKey = reviewDialogSourceKey(sessionId, selectedPRKey); + const { selectedFile, filter, setSelectedFile, setFilter } = + useReviewDialogTransientState(reviewSourceKey); const [splitView, setSplitView] = useState(() => typeof window === "undefined" ? false : localStorage.getItem("diff-view-mode") === "split", ); @@ -362,7 +375,6 @@ function useReviewDialogState(props: ReviewDialogProps) { }, [getStorePendingComments, sessionId]); const markCommentsSent = useCommentsStore((s) => s.markCommentsSent); - const [filter, setFilter] = useState(""); const allFiles = useMemo( () => buildAllFiles(gitStatusFiles, cumulativeDiff, prDiffFiles, prRepoName, useRepositoryKeys), [gitStatusFiles, cumulativeDiff, prDiffFiles, prRepoName, useRepositoryKeys], @@ -389,13 +401,7 @@ function useReviewDialogState(props: ReviewDialogProps) { for (const file of allFiles) refs.set(reviewFileKey(file), createRef()); return refs; }, [allFiles]); - const prevCountRef = useRef(null); - - useEffect(() => { - const prevCount = prevCountRef.current; - if (open && prevCount !== null && prevCount > 0 && allFiles.length === 0) onOpenChange(false); - prevCountRef.current = allFiles.length; - }, [open, allFiles.length, onOpenChange]); + useReviewDialogAutoClose({ open, fileCount: allFiles.length, prDiffLoading, onOpenChange }); const handlers = useReviewDialogHandlers({ allFiles, @@ -412,12 +418,13 @@ function useReviewDialogState(props: ReviewDialogProps) { }, [handlers.handleToggleSplitView], ); - const handleSelectFile = useCallback( - (path: string) => handlers.handleSelectFile(path, setSelectedFile), - [handlers.handleSelectFile], + const handleSelectFile = usePRKeyedReviewFileSelection( + handlers.handleSelectFile, + setSelectedFile, ); return { + reviewSourceKey, selectedFile, splitView, wordWrap, @@ -442,6 +449,8 @@ function useReviewDialogState(props: ReviewDialogProps) { }; } +export type ReviewDialogViewState = ReturnType; + /** * Drives review file-selection from the active walkthrough step: when the tour * advances (or the dialog opens on a tour), select+scroll to the step's file so @@ -487,95 +496,41 @@ function useReviewDialogWalkthroughRequest({ } export const ReviewDialog = memo(function ReviewDialog(props: ReviewDialogProps) { - const { open, onOpenChange, sessionId, baseBranch, onOpenFile } = props; + const { + open, + onOpenChange, + sessionId, + baseBranch, + onOpenFile, + prs = [], + selectedPR = null, + onSelectPR, + prDiffLoading = false, + prDiffError = null, + onRetryPRDiff, + } = props; const s = useReviewDialogState(props); const handleRequestWalkthrough = useReviewDialogWalkthroughRequest({ sessionId, allFiles: s.allFiles, }); - const splitRowRef = useRef(null); - const sidebar = useReviewSidebarResize(splitRowRef, open); useWalkthroughFileSelection(open, s.allFiles, s.filter, s.setFilter, s.handleSelectFile); return ( - - - Review Changes - onOpenChange(false)} - onRequestWalkthrough={handleRequestWalkthrough} - requestWalkthroughDisabled={s.allFiles.length === 0} - getPendingComments={s.getPendingComments} - markCommentsSent={s.markCommentsSent} - /> -
-
- -
- -
- {s.filteredFiles.length > 0 ? ( - - ) : ( -
- {s.filter.trim() ? "No files match the filter" : "No changes to review"} -
- )} -
-
- - + ); }); diff --git a/apps/web/components/review/review-pr-selector.tsx b/apps/web/components/review/review-pr-selector.tsx new file mode 100644 index 0000000000..a45fd3f0f2 --- /dev/null +++ b/apps/web/components/review/review-pr-selector.tsx @@ -0,0 +1,109 @@ +"use client"; + +import { IconChevronDown, IconGitPullRequest, IconLoader2 } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuLabel, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, + DropdownMenuSeparator, + DropdownMenuTrigger, +} from "@kandev/ui/dropdown-menu"; +import { cn } from "@kandev/ui/lib/utils"; +import { prIdentitySlug, prTaskKey } from "@/components/github/pr-utils"; +import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import type { TaskPR } from "@/lib/types/github"; + +type ReviewPRSelectorProps = { + prs: TaskPR[]; + selectedPR: TaskPR | null; + loading: boolean; + onSelectPR: (pr: TaskPR) => void; + className?: string; + compact?: boolean; + testIdPrefix?: string; +}; + +export function ReviewPRSelector({ + prs, + selectedPR, + loading, + onSelectPR, + className, + compact = false, + testIdPrefix = "review-pr-selector", +}: ReviewPRSelectorProps) { + const { isMobile, isFinePointer } = useResponsiveBreakpoint(); + if (prs.length < 2 || !selectedPR) return null; + + const selectedKey = prTaskKey(selectedPR); + const touchSized = isMobile || !isFinePointer; + let triggerHeight = "min-h-8"; + if (touchSized) triggerHeight = "min-h-11"; + else if (compact) triggerHeight = "min-h-5"; + + return ( + + + + + + Review pull request + + { + const nextPR = prs.find((pr) => prTaskKey(pr) === nextKey); + if (nextPR) onSelectPR(nextPR); + }} + > + {prs.map((pr) => ( + + + + + {pr.repo} #{pr.pr_number} + + {pr.pr_title} + + + ))} + + + + ); +} diff --git a/apps/web/components/review/review-repository-identity.test.ts b/apps/web/components/review/review-repository-identity.test.ts new file mode 100644 index 0000000000..9ffd9e1d9d --- /dev/null +++ b/apps/web/components/review/review-repository-identity.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from "vitest"; +import { resolvePRReviewRepositoryIdentity } from "./review-repository-identity"; + +const REPOSITORY_ID = "repo-widgets"; +const REPOSITORY_NAME = "widgets"; +const PRIMARY_BRANCH = "feat/first"; +const SIBLING_BRANCH = "feat/second"; +const COLLIDING_BRANCH = "feature-foo"; + +const primaryPR = { + repository_id: REPOSITORY_ID, + repo: REPOSITORY_NAME, + head_branch: PRIMARY_BRANCH, +}; + +const siblingPR = { + ...primaryPR, + head_branch: SIBLING_BRANCH, +}; + +const taskRepositories = [ + { + repository_id: REPOSITORY_ID, + base_branch: "main", + checkout_branch: PRIMARY_BRANCH, + position: 0, + }, + { + repository_id: REPOSITORY_ID, + base_branch: "main", + checkout_branch: SIBLING_BRANCH, + position: 1, + }, +]; + +const worktrees = [ + { + repositoryId: REPOSITORY_ID, + branchSlug: "feat-first", + branch: PRIMARY_BRANCH, + path: "/tasks/example/widgets", + position: 0, + }, + { + repositoryId: REPOSITORY_ID, + branchSlug: "feat-second", + branch: SIBLING_BRANCH, + path: "/tasks/example/widgets-feat-second", + position: 1, + }, +]; + +describe("resolvePRReviewRepositoryIdentity", () => { + it("uses the selected sibling worktree subpath for same-repo multi-branch review", () => { + expect( + resolvePRReviewRepositoryIdentity({ + pr: siblingPR, + workspaceRepositoryName: REPOSITORY_NAME, + taskRepositories, + worktrees, + }), + ).toBe("widgets-feat-second"); + }); + + it("keeps the flat worktree identity for the primary branch", () => { + expect( + resolvePRReviewRepositoryIdentity({ + pr: primaryPR, + workspaceRepositoryName: REPOSITORY_NAME, + taskRepositories, + worktrees, + }), + ).toBe(REPOSITORY_NAME); + }); + + it("derives the sibling identity while live worktree metadata is still hydrating", () => { + expect( + resolvePRReviewRepositoryIdentity({ + pr: siblingPR, + workspaceRepositoryName: REPOSITORY_NAME, + taskRepositories, + worktrees: [], + }), + ).toBe("widgets-feat-second"); + }); + + it("prefers the exact branch when normalized branch slugs collide", () => { + expect( + resolvePRReviewRepositoryIdentity({ + pr: { ...siblingPR, head_branch: COLLIDING_BRANCH }, + workspaceRepositoryName: REPOSITORY_NAME, + taskRepositories: [ + { ...taskRepositories[0], checkout_branch: "feature/foo" }, + { ...taskRepositories[1], checkout_branch: COLLIDING_BRANCH }, + ], + worktrees: [ + { + ...worktrees[0], + branch: "feature/foo", + branchSlug: COLLIDING_BRANCH, + path: "/tasks/example/widgets-feature-slash-foo", + }, + { + ...worktrees[1], + branch: COLLIDING_BRANCH, + branchSlug: COLLIDING_BRANCH, + path: "/tasks/example/widgets-feature-dash-foo", + }, + ], + }), + ).toBe("widgets-feature-dash-foo"); + }); +}); diff --git a/apps/web/components/review/review-repository-identity.ts b/apps/web/components/review/review-repository-identity.ts new file mode 100644 index 0000000000..a77828245f --- /dev/null +++ b/apps/web/components/review/review-repository-identity.ts @@ -0,0 +1,108 @@ +import { resolvePRReviewRepositoryName, sanitizeReviewRepositoryName } from "./types"; + +type ReviewPRIdentity = { + repository_id?: string; + repo: string; + head_branch?: string; +}; + +export type ReviewTaskRepositoryIdentity = { + repository_id: string; + base_branch?: string; + checkout_branch?: string; + position: number; +}; + +export type ReviewWorktreeIdentity = { + id?: string; + repositoryId?: string; + branchSlug?: string; + branch?: string; + path?: string; + position?: number; +}; + +type ResolvePRReviewRepositoryIdentityInput = { + pr: ReviewPRIdentity | null | undefined; + workspaceRepositoryName?: string | null; + taskRepositories: ReviewTaskRepositoryIdentity[]; + worktrees: ReviewWorktreeIdentity[]; +}; + +function worktreeDirectoryName(path: string | undefined): string | undefined { + const directory = path + ?.replace(/[\\/]+$/, "") + .split(/[\\/]/) + .pop(); + return directory ? sanitizeReviewRepositoryName(directory) || undefined : undefined; +} + +function findPRTaskRepository( + pr: ReviewPRIdentity, + taskRepositories: ReviewTaskRepositoryIdentity[], +): ReviewTaskRepositoryIdentity | undefined { + const matchingRepository = taskRepositories.filter( + (taskRepository) => taskRepository.repository_id === pr.repository_id, + ); + return ( + matchingRepository.find( + (taskRepository) => taskRepository.checkout_branch === pr.head_branch, + ) ?? + matchingRepository.find((taskRepository) => taskRepository.base_branch === pr.head_branch) ?? + (matchingRepository.length === 1 ? matchingRepository[0] : undefined) + ); +} + +function findPRWorktree( + pr: ReviewPRIdentity, + taskRepository: ReviewTaskRepositoryIdentity | undefined, + worktrees: ReviewWorktreeIdentity[], +): ReviewWorktreeIdentity | undefined { + const branchSlug = sanitizeReviewRepositoryName(pr.head_branch ?? ""); + const repositoryWorktrees = worktrees.filter( + (worktree) => worktree.repositoryId === pr.repository_id, + ); + const exactBranch = repositoryWorktrees.find( + (worktree) => pr.head_branch && worktree.branch === pr.head_branch, + ); + if (exactBranch) return exactBranch; + + const exactPosition = repositoryWorktrees.find( + (worktree) => taskRepository !== undefined && worktree.position === taskRepository.position, + ); + if (exactPosition) return exactPosition; + + const slugMatches = repositoryWorktrees.filter( + (worktree) => branchSlug && worktree.branchSlug === branchSlug, + ); + return slugMatches.length === 1 ? slugMatches[0] : undefined; +} + +/** + * Resolves the repository_name stamped by agentctl for a selected PR. Same-repo + * multi-branch tasks use sibling worktree directories (`repo-branch-slug`), so + * the canonical workspace repo name alone is not a stable review identity. + */ +export function resolvePRReviewRepositoryIdentity({ + pr, + workspaceRepositoryName, + taskRepositories, + worktrees, +}: ResolvePRReviewRepositoryIdentityInput): string | undefined { + const canonicalName = resolvePRReviewRepositoryName(pr, workspaceRepositoryName); + if (!pr?.repository_id || !canonicalName) return canonicalName; + + const taskRepository = findPRTaskRepository(pr, taskRepositories); + const worktreeName = worktreeDirectoryName(findPRWorktree(pr, taskRepository, worktrees)?.path); + if (worktreeName) return worktreeName; + + const siblingRepositories = taskRepositories + .filter((candidate) => candidate.repository_id === pr.repository_id) + .sort((left, right) => left.position - right.position); + if (!taskRepository || siblingRepositories.length < 2) return canonicalName; + if (taskRepository === siblingRepositories[0]) return canonicalName; + + const branch = taskRepository.checkout_branch || taskRepository.base_branch || pr.head_branch; + const branchSlug = sanitizeReviewRepositoryName(branch ?? ""); + return branchSlug ? `${canonicalName}-${branchSlug}` : canonicalName; +} diff --git a/apps/web/components/review/review-top-bar.tsx b/apps/web/components/review/review-top-bar.tsx index db55142164..825d9564d3 100644 --- a/apps/web/components/review/review-top-bar.tsx +++ b/apps/web/components/review/review-top-bar.tsx @@ -24,6 +24,8 @@ import { getWebSocketClient } from "@/lib/ws/connection"; import { updateUserSettings } from "@/lib/api"; import { VcsSplitButton } from "@/components/vcs-split-button"; import { FixCommentsButton } from "./review-fix-comments-button"; +import { ReviewPRSelector } from "./review-pr-selector"; +import type { TaskPR } from "@/lib/types/github"; type ReviewTopBarProps = { sessionId: string; @@ -41,6 +43,10 @@ type ReviewTopBarProps = { requestWalkthroughDisabled?: boolean; getPendingComments: () => DiffComment[]; markCommentsSent: (ids: string[]) => void; + prs: TaskPR[]; + selectedPR: TaskPR | null; + onSelectPR?: (pr: TaskPR) => void; + prDiffLoading: boolean; }; function sendAutoMarkSetting(checked: boolean) { @@ -103,6 +109,50 @@ function ReviewProgress({ reviewedCount, totalCount }: ReviewProgressProps) { ); } +function ReviewDisplayControls({ + wordWrap, + splitView, + onToggleWordWrap, + onToggleSplitView, +}: Pick) { + return ( + <> + + + + + Toggle word wrap + + + + + + + {splitView ? "Switch to unified view" : "Switch to split view"} + + + + ); +} + function ReviewWalkthroughButton({ onRequestWalkthrough, disabled, @@ -154,6 +204,10 @@ export const ReviewTopBar = memo(function ReviewTopBar({ requestWalkthroughDisabled, getPendingComments, markCommentsSent, + prs, + selectedPR, + onSelectPR, + prDiffLoading, }: ReviewTopBarProps) { const reviewAutoMarkOnScroll = useAppStore((state) => state.userSettings.reviewAutoMarkOnScroll); const setUserSettings = useAppStore((state) => state.setUserSettings); @@ -175,44 +229,27 @@ export const ReviewTopBar = memo(function ReviewTopBar({ ); return ( -
+
+ {onSelectPR && ( + + )} - - - - - Toggle word wrap - - - - - - - {splitView ? "Switch to unified view" : "Switch to split view"} - - + {commentCount > 0 && ( state.tasks.activeTaskId); @@ -154,8 +160,88 @@ function usePerRepoCallbacks( ); } -function useChangesPanelPRData(repositoryNames: string[]) { +type ChangesPanelPRBuildInput = { + prs: TaskPR[]; + filesByPRKey: Record; + repoNameById: Record; + taskHasMultipleRepos: boolean; + selectedPRId: string | undefined; + selectedPRRepositoryName: string | undefined; + useRepositoryKeys: boolean; +}; + +function countPRsByRepository(prs: TaskPR[]): Map { + const counts = new Map(); + for (const pr of prs) { + const repositoryId = pr.repository_id ?? ""; + counts.set(repositoryId, (counts.get(repositoryId) ?? 0) + 1); + } + return counts; +} + +function repositoryNameForPR(pr: TaskPR, repoNameById: Record): string { + if (!pr.repository_id) return ""; + return repoNameById[pr.repository_id] ?? ""; +} + +function progressRepositoryName( + pr: TaskPR, + selectedPRId: string | undefined, + selectedPRRepositoryName: string | undefined, + workspaceRepositoryName: string, +): string { + if (pr.id === selectedPRId && selectedPRRepositoryName) return selectedPRRepositoryName; + return resolvePRReviewRepositoryName(pr, workspaceRepositoryName || undefined) ?? pr.repo; +} + +function buildChangesPanelPRData({ + prs, + filesByPRKey, + repoNameById, + taskHasMultipleRepos, + selectedPRId, + selectedPRRepositoryName, + useRepositoryKeys, +}: ChangesPanelPRBuildInput): { prFiles: PRChangedFile[]; prDiffFiles: PRDiffFile[] } { + const merged: PRChangedFile[] = []; + const progressSources = new Map(); + const prCounts = countPRsByRepository(prs); + const anyRepoMultiPR = Array.from(prCounts.values()).some((count) => count > 1); + const needsStamp = prs.length > 1; + + for (const pr of prs) { + const files = filesByPRKey[prFetchKey(pr)] ?? []; + const repositoryName = repositoryNameForPR(pr, repoNameById); + const stamp = computePRGroupStamp({ + needsStamp, + taskHasMultipleRepos, + anyRepoMultiPR, + repoName: repositoryName || `${pr.owner}/${pr.repo}`, + branch: pr.head_branch ?? "", + prNumber: pr.pr_number, + }); + merged.push(...mapPRFilesToChangedFiles(files, stamp, prTaskKey(pr))); + progressSources.set(pr.id, { + repositoryName: progressRepositoryName( + pr, + selectedPRId, + selectedPRRepositoryName, + repositoryName, + ), + files, + }); + } + + return { + prFiles: merged, + prDiffFiles: selectPRFilesForReviewProgress(progressSources, selectedPRId, useRepositoryKeys), + }; +} + +function useChangesPanelPRData(repositoryNames: string[], sessionId: string | null) { const { prs, filesByPRKey } = useActiveTaskPRsWithFiles(); + const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); + const { selectedPR: taskPR } = useReviewPRSelection(activeTaskId); const reposByWorkspace = useAppStore((s) => s.repositories.itemsByWorkspaceId); const repoNameById = useMemo(() => buildRepoNameById(reposByWorkspace), [reposByWorkspace]); const taskRepositoryCount = useAppStore((s) => { @@ -166,7 +252,7 @@ function useChangesPanelPRData(repositoryNames: string[]) { }); const taskHasMultipleRepos = taskRepositoryCount > 1; const useRepositoryKeys = isReviewMultiRepo(taskRepositoryCount, repositoryNames); - const taskPR = useActiveTaskPR(); + const selectedPRRepositoryName = usePRReviewRepositoryIdentity(activeTaskId, sessionId, taskPR); const refreshKey = taskPR?.last_synced_at ?? null; const { commits: prCommitsList } = usePRCommits( taskPR?.owner ?? null, @@ -174,52 +260,27 @@ function useChangesPanelPRData(repositoryNames: string[]) { taskPR?.pr_number ?? null, refreshKey, ); - const { prFiles, prDiffFiles } = useMemo(() => { - const merged: PRChangedFile[] = []; - const progressSources = new Map(); - // Multi-branch tasks open multiple PRs on the same repo — files must - // split by PR so the agent's two branches don't get conflated under one - // "kandev" header. We stamp each row with a label unique per PR. - // - // - 1 PR total → no stamp (single section header is enough) - // - Multi-repo, 1 PR/repo → stamp = repo name (legacy behavior) - // - Multi-PR per repo → stamp = " · " so the - // group label still names the repo but - // disambiguates the branch the PR is on - // - Single repo + N PRs → stamp = head_branch (repo is redundant) - const prCountByRepoID = new Map(); - for (const pr of prs) { - const key = pr.repository_id ?? ""; - prCountByRepoID.set(key, (prCountByRepoID.get(key) ?? 0) + 1); - } - const anyRepoMultiPR = Array.from(prCountByRepoID.values()).some((n) => n > 1); - const needsStamp = prs.length > 1; - for (const pr of prs) { - const key = `${pr.owner}/${pr.repo}/${pr.pr_number}/${pr.last_synced_at ?? ""}`; - const files = filesByPRKey[key] ?? []; - const repoName = pr.repository_id ? (repoNameById[pr.repository_id] ?? "") : ""; - const branch = pr.head_branch ?? ""; - const stamp = computePRGroupStamp({ - needsStamp, + const { prFiles, prDiffFiles } = useMemo( + () => + buildChangesPanelPRData({ + prs, + filesByPRKey, + repoNameById, taskHasMultipleRepos, - anyRepoMultiPR, - repoName: repoName || `${pr.owner}/${pr.repo}`, - branch, - prNumber: pr.pr_number, - }); - merged.push(...mapPRFilesToChangedFiles(files, stamp)); - progressSources.set(pr.id, { - repositoryName: resolvePRReviewRepositoryName(pr, repoName || undefined) ?? pr.repo, - files, - }); - } - const progressFiles = selectPRFilesForReviewProgress( - progressSources, + selectedPRId: taskPR?.id, + selectedPRRepositoryName, + useRepositoryKeys, + }), + [ + prs, + filesByPRKey, + repoNameById, + taskHasMultipleRepos, taskPR?.id, + selectedPRRepositoryName, useRepositoryKeys, - ); - return { prFiles: merged, prDiffFiles: progressFiles }; - }, [prs, filesByPRKey, repoNameById, taskHasMultipleRepos, taskPR?.id, useRepositoryKeys]); + ], + ); const hasPRFiles = prFiles.length > 0; const hasPRCommits = prCommitsList.length > 0; return { @@ -246,7 +307,7 @@ export function useChangesPanelData() { () => [...git.repoNames, ...getCumulativeReviewRepositoryNames(git.cumulativeDiff?.files)], [git.repoNames, git.cumulativeDiff], ); - const prData = useChangesPanelPRData(reviewRepositoryNames); + const prData = useChangesPanelPRData(reviewRepositoryNames, activeSessionId); const vcsDialogs = useVcsDialogs(); const baseBranchDisplay = useMemo(() => getBaseBranchDisplay(baseBranch), [baseBranch]); const unstagedFiles = useMemo(() => mapToChangedFiles(git.unstagedFiles), [git.unstagedFiles]); diff --git a/apps/web/components/task/changes-panel-helpers.test.ts b/apps/web/components/task/changes-panel-helpers.test.ts index 72bbbc713c..237c2acfa3 100644 --- a/apps/web/components/task/changes-panel-helpers.test.ts +++ b/apps/web/components/task/changes-panel-helpers.test.ts @@ -93,6 +93,16 @@ describe("mapPRFilesToChangedFiles", () => { expect(out.every((f) => f.repository_name === "frontend")).toBe(true); }); + it("stamps the exact PR key on every row", () => { + const [out] = mapPRFilesToChangedFiles( + [diffFile({ filename: "README.md" })], + "frontend", + "acme/widgets/42", + ); + + expect(out.prKey).toBe("acme/widgets/42"); + }); + it("defaults repository_name to '' when caller omits it (single-repo path)", () => { // Empty string is meaningful: PRFilesGroupedList treats one group with // empty name as the single-repo case and skips per-repo sub-headers. diff --git a/apps/web/components/task/changes-panel-helpers.ts b/apps/web/components/task/changes-panel-helpers.ts index 5d86b64c59..b1614b3180 100644 --- a/apps/web/components/task/changes-panel-helpers.ts +++ b/apps/web/components/task/changes-panel-helpers.ts @@ -207,9 +207,11 @@ export function computeStagedStats(stagedFiles: FileInfo[]) { export function mapPRFilesToChangedFiles( files: PRDiffFile[], repositoryName?: string, + prKey?: string, ): PRChangedFile[] { return files.map((file) => ({ path: file.filename, + prKey, status: normalizeFileChangeStatus(file.status), plus: file.additions, minus: file.deletions, diff --git a/apps/web/components/task/changes-panel-pr-files.test.tsx b/apps/web/components/task/changes-panel-pr-files.test.tsx index aba469af6d..fcc9ae6ba1 100644 --- a/apps/web/components/task/changes-panel-pr-files.test.tsx +++ b/apps/web/components/task/changes-panel-pr-files.test.tsx @@ -17,6 +17,7 @@ describe("PRFilesGroupedList", () => { minus: 0, oldPath: undefined, repository_name: "backend", + prKey: "acme/backend/42", }, ]} onOpenDiff={onOpenDiff} @@ -27,6 +28,7 @@ describe("PRFilesGroupedList", () => { expect(onOpenDiff).toHaveBeenCalledWith("README.md", { source: "pr", repositoryName: "backend", + prKey: "acme/backend/42", }); }); diff --git a/apps/web/components/task/changes-panel-pr-files.tsx b/apps/web/components/task/changes-panel-pr-files.tsx index 684fe7c722..c9caffeba7 100644 --- a/apps/web/components/task/changes-panel-pr-files.tsx +++ b/apps/web/components/task/changes-panel-pr-files.tsx @@ -120,6 +120,7 @@ function PRFileRow({ return (
  • diff --git a/apps/web/components/task/changes-panel-timeline.tsx b/apps/web/components/task/changes-panel-timeline.tsx index 2655bea8c3..02279e43c9 100644 --- a/apps/web/components/task/changes-panel-timeline.tsx +++ b/apps/web/components/task/changes-panel-timeline.tsx @@ -519,6 +519,8 @@ export function FileListSection(props: FileListSectionProps) { export type PRChangedFile = { path: string; + /** Stable owner/repo/number identity for the PR that produced this row. */ + prKey?: string; status: FileInfo["status"]; plus: number | undefined; minus: number | undefined; diff --git a/apps/web/components/task/changes-top-bar.tsx b/apps/web/components/task/changes-top-bar.tsx index 8ca807bbd0..0b24a10cd5 100644 --- a/apps/web/components/task/changes-top-bar.tsx +++ b/apps/web/components/task/changes-top-bar.tsx @@ -18,7 +18,9 @@ import { DropdownMenuTrigger, } from "@kandev/ui/dropdown-menu"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; -import { PanelHeaderBarSplit } from "./panel-primitives"; +import { ReviewPRSelector } from "@/components/review/review-pr-selector"; +import type { TaskPR } from "@/lib/types/github"; +import { PanelHeaderBar, PanelHeaderBarSplit } from "./panel-primitives"; export type ChangesTopBarProps = { autoMarkOnScroll: boolean; @@ -34,6 +36,10 @@ export type ChangesTopBarProps = { handleFixComments: () => void; handleRequestWalkthrough?: () => void; requestWalkthroughDisabled?: boolean; + prs: TaskPR[]; + selectedPR: TaskPR | null; + prDiffLoading: boolean; + onSelectPR: (pr: TaskPR) => void; }; function ChangesTopBarLeft({ @@ -220,7 +226,53 @@ export function ChangesTopBar({ handleFixComments, handleRequestWalkthrough, requestWalkthroughDisabled, + prs, + selectedPR, + prDiffLoading, + onSelectPR, }: ChangesTopBarProps) { + const selector = ( + + ); + + if (prs.length > 1 && selectedPR) { + return ( + +
    {selector}
    +
    + +
    +
    +
    + +
    + + ); + } + return ( (effectiveEnvId); const hasDevScript = Boolean(repository?.dev_script?.trim()); - const review = useReviewDialog(effectiveSessionId); - const userDefaultLayout = useSyncUserDefaultLayout(); useLspFileOpener(); useEditorKeybinds(); @@ -436,12 +432,7 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ /> - - +
    ); }); diff --git a/apps/web/components/task/dockview-panel-content.diff.test.tsx b/apps/web/components/task/dockview-panel-content.diff.test.tsx index c86cde5d32..520c67b682 100644 --- a/apps/web/components/task/dockview-panel-content.diff.test.tsx +++ b/apps/web/components/task/dockview-panel-content.diff.test.tsx @@ -44,6 +44,7 @@ vi.mock("@/components/state-provider", () => ({ })); import { renderPanel } from "./dockview-panel-content"; +import { renderPanel as renderSharedPanel } from "./dockview-shared"; describe("dockview diff panel content", () => { it("passes repositoryName from panel params into the file-mode diff view", () => { @@ -54,6 +55,7 @@ describe("dockview diff panel content", () => { path: "README.md", source: "uncommitted", repositoryName: "frontend", + prKey: "acme/frontend/42", })} , ); @@ -63,6 +65,28 @@ describe("dockview diff panel content", () => { filePath: "README.md", sourceFilter: "uncommitted", fileRepositoryName: "frontend", + prKey: "acme/frontend/42", + }); + }); + + it("preserves the PR source in the shared dockview renderer", () => { + render( + <> + {renderSharedPanel("preview:file-diff", "diff-viewer", { + kind: "file", + path: "README.md", + source: "pr", + repositoryName: "widgets · feat/second", + prKey: "acme/widgets/42", + })} + , + ); + + expect(taskChangesPanel.props).toMatchObject({ + mode: "file", + filePath: "README.md", + sourceFilter: "pr", + prKey: "acme/widgets/42", }); }); }); diff --git a/apps/web/components/task/dockview-panel-content.tsx b/apps/web/components/task/dockview-panel-content.tsx index 8c3a3998ec..354add6128 100644 --- a/apps/web/components/task/dockview-panel-content.tsx +++ b/apps/web/components/task/dockview-panel-content.tsx @@ -95,6 +95,7 @@ function DiffViewerContent({ const selectedPath = panelKind === "file" ? (params?.path as string) : undefined; const selectedRepositoryName = panelKind === "file" ? (params?.repositoryName as string | undefined) : undefined; + const selectedPRKey = panelKind === "file" ? (params?.prKey as string | undefined) : undefined; const sourceFilter = ((params?.source as string) || "all") as "all" | ReviewSource; const panelSelectedDiff = panelKind === "all" ? selectedDiff : null; const handleClosePanel = useCallback(() => { @@ -108,6 +109,7 @@ function DiffViewerContent({ mode={panelKind as "all" | "file"} filePath={selectedPath} fileRepositoryName={selectedRepositoryName} + prKey={selectedPRKey} sourceFilter={sourceFilter} selectedDiff={panelSelectedDiff} onClearSelected={() => setSelectedDiff(null)} @@ -139,7 +141,11 @@ function ChangesContent({ panelId }: { panelId: string }) { ); const handleOpenDiffFile = useCallback( (path: string, options?: OpenDiffOptions) => - addFileDiffPanel(path, { source: options?.source, repositoryName: options?.repositoryName }), + addFileDiffPanel(path, { + source: options?.source, + repositoryName: options?.repositoryName, + prKey: options?.prKey, + }), [addFileDiffPanel], ); const handleOpenCommitDetail = useCallback( diff --git a/apps/web/components/task/dockview-review-dialog.tsx b/apps/web/components/task/dockview-review-dialog.tsx index 3ba70ad1fb..7d19bb7738 100644 --- a/apps/web/components/task/dockview-review-dialog.tsx +++ b/apps/web/components/task/dockview-review-dialog.tsx @@ -1,31 +1,53 @@ "use client"; import { ReviewDialog } from "@/components/review/review-dialog"; -import type { useReviewDialog } from "./use-review-dialog"; +import { WalkthroughOverlay } from "@/components/review/walkthrough-overlay"; +import { useReviewDialog } from "./use-review-dialog"; -type DockviewReviewDialogProps = { +type TaskReviewDialogMountProps = { sessionId: string | null; - review: ReturnType; + taskId: string | null; + onSelectWalkthroughFile?: (path: string, repo?: string) => void; }; -export function DockviewReviewDialog({ sessionId, review }: DockviewReviewDialogProps) { +export function TaskReviewDialogMount({ + sessionId, + taskId, + onSelectWalkthroughFile, +}: TaskReviewDialogMountProps) { + const review = useReviewDialog(sessionId); + if (!sessionId) { return null; } return ( - + <> + + + ); } diff --git a/apps/web/components/task/dockview-session-tabs.ts b/apps/web/components/task/dockview-session-tabs.ts index 822b83e769..1e8f4bed27 100644 --- a/apps/web/components/task/dockview-session-tabs.ts +++ b/apps/web/components/task/dockview-session-tabs.ts @@ -16,7 +16,7 @@ import { createDebugLogger, isDebug } from "@/lib/debug/log"; import type { TaskSession } from "@/lib/types/http"; import type { TaskPR } from "@/lib/types/github"; import { getPrimaryTaskPR } from "@/hooks/domains/github/use-task-pr"; -import { prTaskKey } from "@/components/github/pr-detail-panel"; +import { prTaskKey } from "@/components/github/pr-utils"; import { shouldActivateSessionPanel, shouldPreserveActivePanel, diff --git a/apps/web/components/task/dockview-shared.tsx b/apps/web/components/task/dockview-shared.tsx index b28cd02ce2..311eb21920 100644 --- a/apps/web/components/task/dockview-shared.tsx +++ b/apps/web/components/task/dockview-shared.tsx @@ -12,6 +12,7 @@ import { useFileEditors } from "@/hooks/use-file-editors"; import { useSessionGitStatus } from "@/hooks/domains/session/use-session-git-status"; import { useSessionCommits } from "@/hooks/domains/session/use-session-commits"; import { useEnvironmentSessionId } from "@/hooks/use-environment-session-id"; +import type { ReviewSource } from "@/hooks/domains/session/use-review-sources"; // Panel components (rendered via portals, not directly by dockview) import { TaskChatPanel } from "./task-chat-panel"; @@ -262,6 +263,8 @@ function DiffViewerContent({ const selectedPath = panelKind === "file" ? (params?.path as string) : undefined; const selectedRepositoryName = panelKind === "file" ? (params?.repositoryName as string | undefined) : undefined; + const selectedPRKey = panelKind === "file" ? (params?.prKey as string | undefined) : undefined; + const sourceFilter = ((params?.source as string) || "all") as "all" | ReviewSource; const panelSelectedDiff = panelKind === "all" ? selectedDiff : null; useResyncGitStatusOnTabActivate(panelId, activeSessionId); const handleClosePanel = useCallback(() => { @@ -275,6 +278,8 @@ function DiffViewerContent({ mode={panelKind as "all" | "file"} filePath={selectedPath} fileRepositoryName={selectedRepositoryName} + prKey={selectedPRKey} + sourceFilter={sourceFilter} selectedDiff={panelSelectedDiff} onClearSelected={() => setSelectedDiff(null)} onOpenFile={openFile} @@ -308,7 +313,11 @@ function ChangesContent({ panelId }: { panelId: string }) { ); const handleOpenDiffFile = useCallback( (path: string, options?: OpenDiffOptions) => - addFileDiffPanel(path, { source: options?.source, repositoryName: options?.repositoryName }), + addFileDiffPanel(path, { + source: options?.source, + repositoryName: options?.repositoryName, + prKey: options?.prKey, + }), [addFileDiffPanel], ); const handleOpenCommitDetail = useCallback( diff --git a/apps/web/components/task/mobile/mobile-changes-panel.tsx b/apps/web/components/task/mobile/mobile-changes-panel.tsx index 95c12e2960..9c12ccadfd 100644 --- a/apps/web/components/task/mobile/mobile-changes-panel.tsx +++ b/apps/web/components/task/mobile/mobile-changes-panel.tsx @@ -75,6 +75,7 @@ export const MobileChangesPanel = memo(function MobileChangesPanel({ path, sourceFilter: options?.source ?? "all", repositoryName: options?.repositoryName || undefined, + prKey: options?.prKey, }); }, []); diff --git a/apps/web/components/task/mobile/mobile-diff-sheet.tsx b/apps/web/components/task/mobile/mobile-diff-sheet.tsx index b757b39524..3f8311bd50 100644 --- a/apps/web/components/task/mobile/mobile-diff-sheet.tsx +++ b/apps/web/components/task/mobile/mobile-diff-sheet.tsx @@ -166,12 +166,14 @@ function renderPanel( const panelMode = mode.kind; const filePath = mode.kind === "file" ? mode.path : undefined; const fileRepositoryName = mode.kind === "file" ? mode.repositoryName : undefined; + const prKey = mode.kind === "file" ? mode.prKey : undefined; const effectiveSourceFilter = mode.kind === "all" ? activeSource : (mode.sourceFilter ?? "all"); return ( ; - activeTaskId: string | null; - onSelectWalkthroughFile: (path: string, repo?: string) => void; -}) { - if (!sessionId) return null; - return ( - <> - - - - ); -} - export function useMobilePanelHandlers({ effectiveSessionId, handlePanelChange, @@ -432,7 +394,6 @@ export const SessionMobileLayout = memo(function SessionMobileLayout( const { selectedFile, handleOpenFileFromChat, handleOpenFile, handlePanelChangeAndClearSheet } = useMobilePanelHandlers({ effectiveSessionId, handlePanelChange }); - const review = useReviewDialog(effectiveSessionId); const mobileMR = useMobileMRSelection( activeTaskId, effectiveSessionId, @@ -490,10 +451,9 @@ export const SessionMobileLayout = memo(function SessionMobileLayout( presentation="drawer" /> -
    • diff --git a/apps/web/components/task/mobile/session-tablet-layout.tsx b/apps/web/components/task/mobile/session-tablet-layout.tsx index 547c32cc61..1fa9b9f8df 100644 --- a/apps/web/components/task/mobile/session-tablet-layout.tsx +++ b/apps/web/components/task/mobile/session-tablet-layout.tsx @@ -14,6 +14,7 @@ import { useSessionLayoutState } from "@/hooks/use-session-layout-state"; import type { Repository } from "@/lib/types/http"; import type { Layout } from "react-resizable-panels"; import type { OpenFileTab } from "@/lib/types/backend"; +import { TaskReviewDialogMount } from "../dockview-review-dialog"; const DEFAULT_TABLET_LAYOUT: Record = { left: 60, @@ -104,6 +105,7 @@ export const SessionTabletLayout = memo(function SessionTabletLayout({ }: SessionTabletLayoutProps) { // Use shared layout state hook const { + activeTaskId, effectiveSessionId, sessionKey, selectedDiff, @@ -188,6 +190,7 @@ export const SessionTabletLayout = memo(function SessionTabletLayout({ workspaceId={workspaceId} workflowId={workflowId} /> +
    ); }); diff --git a/apps/web/components/task/task-changes-panel-state.ts b/apps/web/components/task/task-changes-panel-state.ts new file mode 100644 index 0000000000..2a29e40532 --- /dev/null +++ b/apps/web/components/task/task-changes-panel-state.ts @@ -0,0 +1,195 @@ +"use client"; + +import { useEffect, useMemo, useRef, type MutableRefObject, type RefObject } from "react"; +import { reviewFileKey, splitReviewFileKey, type ReviewFile } from "@/components/review/types"; +import type { ReviewSource } from "@/hooks/domains/session/use-review-sources"; + +export function shouldDeferReviewStateForPR( + hasSelectedPR: boolean, + prDiffLoading: boolean, + sourceFilter: "all" | ReviewSource, +): boolean { + const usesPRDiff = sourceFilter === "all" || sourceFilter === "pr"; + return hasSelectedPR && prDiffLoading && usesPRDiff; +} + +export function shouldBlockChangesForPR( + sourceFilter: "all" | ReviewSource, + visibleFiles: ReviewFile[], +): boolean { + if (sourceFilter === "pr") return true; + if (sourceFilter !== "all") return false; + return !visibleFiles.some((file) => file.source !== "pr"); +} + +export function resolveSelectedFileRepositoryName( + sourceFilter: "all" | ReviewSource, + prKey: string | undefined, + fileRepositoryName: string | undefined, + visibleFileRepositoryName: string | undefined, +): string | undefined { + return prKey && sourceFilter === "pr" ? visibleFileRepositoryName : fileRepositoryName; +} + +export function shouldCloseFileDiffPanel( + gitStatus: { files?: Record } | undefined, + filePath: string, +): boolean { + if (!gitStatus) return false; + return !gitStatus.files?.[filePath]?.diff; +} + +function shouldCloseFileDiffPanelAggregate( + prevFileSeenRef: MutableRefObject, + gitStatus: { files?: Record } | undefined, + filePath: string, + onBecameEmpty: (() => void) | undefined, +): boolean { + const shouldClose = shouldCloseFileDiffPanel(gitStatus, filePath); + if (prevFileSeenRef.current && shouldClose) { + onBecameEmpty?.(); + return true; + } + if (!shouldClose) prevFileSeenRef.current = true; + return false; +} + +export function useAutoCloseWhenEmpty(opts: { + mode: "all" | "file"; + filePath: string | undefined; + sourceFilter: "all" | ReviewSource; + gitStatus: { files?: Record } | undefined; + visibleCount: number; + prDiffLoading: boolean; + onBecameEmpty: (() => void) | undefined; +}) { + const { mode, filePath, sourceFilter, gitStatus, visibleCount, prDiffLoading, onBecameEmpty } = + opts; + const prevVisibleCountRef = useRef(null); + const prevFileSeenRef = useRef(false); + const prevSourceFilterRef = useRef(null); + + useEffect(() => { + if (!onBecameEmpty) return; + if (prDiffLoading) { + prevVisibleCountRef.current = visibleCount; + return; + } + if (mode === "file" && filePath) { + if (sourceFilter === "all") { + shouldCloseFileDiffPanelAggregate(prevFileSeenRef, gitStatus, filePath, onBecameEmpty); + return; + } + if (prevSourceFilterRef.current !== sourceFilter) { + prevSourceFilterRef.current = sourceFilter; + prevVisibleCountRef.current = null; + } + const prevCount = prevVisibleCountRef.current; + prevVisibleCountRef.current = visibleCount; + if (prevCount !== null && prevCount > 0 && visibleCount === 0) onBecameEmpty(); + return; + } + + if (sourceFilter !== "all") return; + const prevCount = prevVisibleCountRef.current; + if (prevCount !== null && prevCount > 0 && visibleCount === 0) onBecameEmpty(); + prevVisibleCountRef.current = visibleCount; + }, [mode, filePath, sourceFilter, gitStatus, onBecameEmpty, visibleCount, prDiffLoading]); +} + +type FilterVisibleFilesOpts = { + mode: "all" | "file"; + filePath: string | undefined; + fileRepositoryName: string | undefined; + sourceFilter: "all" | ReviewSource; + rawPRFiles?: ReviewFile[]; + prKey?: string; +}; + +export function filterVisibleFiles( + allFiles: ReviewFile[], + opts: FilterVisibleFilesOpts, +): ReviewFile[] { + const { mode, filePath, fileRepositoryName, sourceFilter, rawPRFiles, prKey } = opts; + const repositoryFilter = prKey && sourceFilter === "pr" ? undefined : fileRepositoryName; + if (mode === "file" && filePath && sourceFilter === "pr" && rawPRFiles?.length) { + let prFiles = rawPRFiles.filter((file) => file.path === filePath && file.source === "pr"); + if (repositoryFilter !== undefined) { + prFiles = prFiles.filter((file) => (file.repository_name ?? "") === repositoryFilter); + } + if (prFiles.length > 0) return prFiles; + } + + let files = allFiles; + if (mode === "file" && filePath) { + files = files.filter((file) => file.path === filePath); + if (repositoryFilter !== undefined) { + files = files.filter((file) => (file.repository_name ?? "") === repositoryFilter); + } + } + if (sourceFilter !== "all") files = files.filter((file) => file.source === sourceFilter); + return files; +} + +export function useVisibleDiffState(opts: { + allFiles: ReviewFile[]; + rawPRFiles: ReviewFile[]; + mode: "all" | "file"; + filePath: string | undefined; + fileRepositoryName: string | undefined; + sourceFilter: "all" | ReviewSource; + prKey: string | undefined; + fileRefs: Map>; + reviewedFiles: Set; + staleFiles: Set; +}) { + const { + allFiles, + rawPRFiles, + mode, + filePath, + fileRepositoryName, + sourceFilter, + prKey, + fileRefs, + reviewedFiles, + staleFiles, + } = opts; + const visibleFiles = useMemo( + () => + filterVisibleFiles(allFiles, { + mode, + filePath, + fileRepositoryName, + sourceFilter, + rawPRFiles, + prKey, + }), + [allFiles, mode, filePath, fileRepositoryName, sourceFilter, rawPRFiles, prKey], + ); + const repositoryFilter = prKey && sourceFilter === "pr" ? undefined : fileRepositoryName; + const visibleFileRefs = useMemo(() => { + if (mode !== "file" || !filePath) return fileRefs; + const refs = new Map>(); + for (const [key, ref] of fileRefs.entries()) { + const split = splitReviewFileKey(key); + if (split.path !== filePath) continue; + if (repositoryFilter !== undefined && (split.repositoryName || "") !== repositoryFilter) { + continue; + } + refs.set(key, ref); + } + return refs; + }, [mode, filePath, fileRefs, repositoryFilter]); + const reviewedCount = useMemo( + () => + visibleFiles.reduce((count, file) => { + const key = reviewFileKey(file); + return !staleFiles.has(key) && reviewedFiles.has(key) ? count + 1 : count; + }, 0), + [visibleFiles, reviewedFiles, staleFiles], + ); + const totalCount = visibleFiles.length; + const progressPercent = totalCount > 0 ? (reviewedCount / totalCount) * 100 : 0; + return { visibleFiles, visibleFileRefs, reviewedCount, totalCount, progressPercent }; +} diff --git a/apps/web/components/task/task-changes-panel.test.ts b/apps/web/components/task/task-changes-panel.test.ts index bd86aac64b..dcfc4cdcd9 100644 --- a/apps/web/components/task/task-changes-panel.test.ts +++ b/apps/web/components/task/task-changes-panel.test.ts @@ -1,9 +1,14 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { scrollToFileAndClear } from "./task-changes-panel"; import { - shouldCloseFileDiffPanel, filterVisibleFiles, - scrollToFileAndClear, -} from "./task-changes-panel"; + resolveSelectedFileRepositoryName, + shouldBlockChangesForPR, + shouldDeferReviewStateForPR, + shouldCloseFileDiffPanel, + useAutoCloseWhenEmpty, +} from "./task-changes-panel-state"; import type { ReviewFile } from "@/components/review/types"; const PATH = "src/foo.ts"; @@ -161,6 +166,87 @@ describe("filterVisibleFiles", () => { expect(result[0].repository_name).toBe("frontend"); expect(result[0].source).toBe("pr"); }); + + it("exact PR identity wins when the timeline group label is not the repository name", () => { + const rawPRFiles = [{ ...file("README.md", "pr"), repository_name: "widgets" }]; + const result = filterVisibleFiles( + [], + fileOpts("README.md", "pr", "widgets · feat/second", { + rawPRFiles, + prKey: "acme/widgets/42", + }), + ); + + expect(result).toEqual(rawPRFiles); + }); +}); + +describe("shouldDeferReviewStateForPR", () => { + it("defers review marks while the visible source is waiting for PR files", () => { + expect(shouldDeferReviewStateForPR(true, true, "all")).toBe(true); + expect(shouldDeferReviewStateForPR(true, true, "pr")).toBe(true); + }); + + it("keeps local review marks stable during an unrelated PR fetch", () => { + expect(shouldDeferReviewStateForPR(true, true, "uncommitted")).toBe(false); + expect(shouldDeferReviewStateForPR(true, true, "committed")).toBe(false); + }); +}); + +describe("shouldBlockChangesForPR", () => { + it("keeps local files visible while the selected PR is loading or failed", () => { + expect(shouldBlockChangesForPR("all", [file("local.ts", "uncommitted")])).toBe(false); + }); + + it("blocks an all-source view when only PR files are visible", () => { + expect(shouldBlockChangesForPR("all", [file("pr.ts", "pr")])).toBe(true); + }); + + it("blocks a PR-only view until the selected PR is ready", () => { + expect(shouldBlockChangesForPR("pr", [])).toBe(true); + }); +}); + +describe("resolveSelectedFileRepositoryName", () => { + it("uses the selected PR file identity instead of a timeline group label", () => { + expect( + resolveSelectedFileRepositoryName( + "pr", + "acme/widgets/42", + "widgets · feature/second", + "widgets-feature-second", + ), + ).toBe("widgets-feature-second"); + }); + + it("falls back to the router repository name for non-PR source views", () => { + expect(resolveSelectedFileRepositoryName("uncommitted", undefined, "backend", "widgets")).toBe( + "backend", + ); + }); +}); + +describe("useAutoCloseWhenEmpty", () => { + it("keeps a PR-backed panel open across a loading-to-empty transition", () => { + const onBecameEmpty = vi.fn(); + const initialProps = { + mode: "all" as const, + filePath: undefined, + sourceFilter: "all" as const, + gitStatus: undefined, + visibleCount: 1, + prDiffLoading: false, + onBecameEmpty, + }; + const { rerender } = renderHook((props) => useAutoCloseWhenEmpty(props), { + initialProps, + }); + + rerender({ ...initialProps, visibleCount: 0, prDiffLoading: true }); + rerender({ ...initialProps, visibleCount: 0, prDiffLoading: false }); + + expect(onBecameEmpty).not.toHaveBeenCalled(); + }); }); describe("scrollToFileAndClear", () => { diff --git a/apps/web/components/task/task-changes-panel.tsx b/apps/web/components/task/task-changes-panel.tsx index c9e7b9283b..dfcd71bcef 100644 --- a/apps/web/components/task/task-changes-panel.tsx +++ b/apps/web/components/task/task-changes-panel.tsx @@ -6,7 +6,6 @@ import { TruncatedFilesBanner, useSelectedFileKey } from "./changes-panel-banner import { useToast } from "@/components/toast-provider"; import { useAppStore } from "@/components/state-provider"; import { useReviewSources, type ReviewSource } from "@/hooks/domains/session/use-review-sources"; -import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr"; import { useGitOperations } from "@/hooks/use-git-operations"; import { useSessionFileReviews } from "@/hooks/use-session-file-reviews"; import { useCommentsStore, isDiffComment } from "@/lib/state/slices/comments"; @@ -14,12 +13,20 @@ import { getWebSocketClient } from "@/lib/ws/connection"; import { updateUserSettings } from "@/lib/api"; import { formatReviewCommentsAsMarkdown } from "@/lib/state/slices/comments/format"; import { ReviewDiffList } from "@/components/review/review-diff-list"; +import { ReviewPRDiffBoundary } from "@/components/review/review-dialog-pr-state"; import { DEFAULT_DIFF_WORD_WRAP } from "@/components/diff/diff-defaults"; import type { ReviewFile } from "@/components/review/types"; import { hashDiff, reviewFileKey, splitReviewFileKey } from "@/components/review/types"; import { usePanelActions } from "@/hooks/use-panel-actions"; import { useRequestChangesWalkthrough } from "@/hooks/domains/session/use-request-changes-walkthrough"; import { ChangesTopBar } from "./changes-top-bar"; +import { + resolveSelectedFileRepositoryName, + shouldBlockChangesForPR, + shouldDeferReviewStateForPR, + useAutoCloseWhenEmpty, + useVisibleDiffState, +} from "./task-changes-panel-state"; import type { SelectedDiff } from "./task-layout"; import { useIsTaskArchived, ArchivedPanelPlaceholder } from "./task-archived-context"; @@ -27,6 +34,7 @@ type TaskChangesPanelProps = { mode?: "all" | "file"; filePath?: string; fileRepositoryName?: string; + prKey?: string; selectedDiff: SelectedDiff | null; onClearSelected: () => void; onBecameEmpty?: () => void; @@ -41,16 +49,6 @@ type TaskChangesPanelProps = { wordWrap?: boolean; }; -// Returns true only after gitStatus loads and the file's uncommitted diff is gone. -export function shouldCloseFileDiffPanel( - gitStatus: { files?: Record } | undefined, - filePath: string, -): boolean { - if (!gitStatus) return false; - const entry = gitStatus.files?.[filePath]; - return !entry?.diff; -} - function scrollToFileAndClear( path: string, fileRefs: Map>, @@ -77,11 +75,25 @@ function scrollToFileAndClear( } } -function useChangesView(selectedDiff: SelectedDiff | null, onClearSelected: () => void) { +function useChangesView( + selectedDiff: SelectedDiff | null, + onClearSelected: () => void, + sourceFilter: "all" | ReviewSource, + explicitPRKey?: string, +) { const activeSessionId = useAppStore((state) => state.tasks.activeSessionId); - const { allFiles, cumulativeLoading, prDiffLoading, gitStatus, rawPRFiles, truncatedFilesCount } = - useReviewSources(activeSessionId); - const pr = useActiveTaskPR(); + const reviewSources = useReviewSources(activeSessionId, explicitPRKey); + const { + allFiles, + cumulativeLoading, + prDiffLoading, + prDiffError, + refreshPRDiff, + gitStatus, + rawPRFiles, + truncatedFilesCount, + selectedPR: pr, + } = reviewSources; const { reviews } = useSessionFileReviews(activeSessionId); const byId = useCommentsStore((s) => s.byId); const commentSessionIds = useCommentsStore((s) => @@ -94,7 +106,7 @@ function useChangesView(selectedDiff: SelectedDiff | null, onClearSelected: () = // When a PR exists but its diff files are still loading, the file list // temporarily uses cumulative diff content which has a different hash. // Skip review computation until PR diffs arrive to avoid a 1/1 -> 0/1 flash. - if (pr && prDiffLoading) { + if (shouldDeferReviewStateForPR(Boolean(pr), prDiffLoading, sourceFilter)) { return { reviewedFiles: reviewed, staleFiles: stale }; } for (const file of allFiles) { @@ -109,7 +121,7 @@ function useChangesView(selectedDiff: SelectedDiff | null, onClearSelected: () = } } return { reviewedFiles: reviewed, staleFiles: stale }; - }, [allFiles, reviews, pr, prDiffLoading]); + }, [allFiles, reviews, pr, prDiffLoading, sourceFilter]); const totalCommentCount = useMemo(() => { if (!commentSessionIds || commentSessionIds.length === 0) return 0; @@ -152,8 +164,13 @@ function useChangesView(selectedDiff: SelectedDiff | null, onClearSelected: () = fileRefs, cumulativeLoading, prDiffLoading, + prDiffError, + refreshPRDiff, gitStatus, truncatedFilesCount, + prs: reviewSources.prs, + selectedPR: reviewSources.selectedPR, + selectPR: reviewSources.selectPR, }; } @@ -178,6 +195,29 @@ function useWalkthroughRequest(activeSessionId: string | null | undefined, allFi }); } +function useChangesPRPresentation(opts: { + sourceFilter: "all" | ReviewSource; + prKey: string | undefined; + mode: "all" | "file"; + filePath: string | undefined; + fileRepositoryName: string | undefined; + visibleFiles: ReviewFile[]; +}) { + const visibleSelectedFile = opts.visibleFiles.find((file) => file.path === opts.filePath); + const selectedFileKey = useSelectedFileKey( + opts.mode, + opts.filePath, + resolveSelectedFileRepositoryName( + opts.sourceFilter, + opts.prKey, + opts.fileRepositoryName, + visibleSelectedFile?.repository_name, + ), + ); + const blockChangesForPR = shouldBlockChangesForPR(opts.sourceFilter, opts.visibleFiles); + return { selectedFileKey, blockChangesForPR }; +} + function useChangesActions( activeSessionId: string | null | undefined, allFiles: ReviewFile[], @@ -286,167 +326,11 @@ function useChangesActions( }; } -function shouldCloseFileDiffPanelAggregate( - prevFileSeenRef: React.MutableRefObject, - gitStatus: { files?: Record } | undefined, - filePath: string, - onBecameEmpty: (() => void) | undefined, -): boolean { - const shouldClose = shouldCloseFileDiffPanel(gitStatus, filePath); - if (prevFileSeenRef.current && shouldClose) { - onBecameEmpty?.(); - return true; - } - if (!shouldClose) prevFileSeenRef.current = true; - return false; -} - -function useAutoCloseWhenEmpty(opts: { - mode: "all" | "file"; - filePath: string | undefined; - sourceFilter: "all" | ReviewSource; - gitStatus: { files?: Record } | undefined; - visibleCount: number; - onBecameEmpty: (() => void) | undefined; -}) { - const { mode, filePath, sourceFilter, gitStatus, visibleCount, onBecameEmpty } = opts; - const prevVisibleCountRef = useRef(null); - const prevFileSeenRef = useRef(false); - const prevSourceFilterRef = useRef(null); - - useEffect(() => { - if (!onBecameEmpty) return; - if (mode === "file" && filePath) { - if (sourceFilter === "all") { - shouldCloseFileDiffPanelAggregate(prevFileSeenRef, gitStatus, filePath, onBecameEmpty); - return; - } - // File-mode with explicit source (uncommitted/committed/pr): close when - // that source-specific view becomes empty for the opened file. - // Reset tracking when the filtered source changes so a tab switch - // doesn't fire onBecameEmpty just because the new tab starts empty. - if (prevSourceFilterRef.current !== sourceFilter) { - prevSourceFilterRef.current = sourceFilter; - prevVisibleCountRef.current = null; - } - const prevCount = prevVisibleCountRef.current; - prevVisibleCountRef.current = visibleCount; - if (prevCount !== null && prevCount > 0 && visibleCount === 0) { - onBecameEmpty(); - } - return; - } - - // In list mode, a filtered tab going empty is not a "panel is empty" - // signal — other sources may still have content. - if (sourceFilter !== "all") return; - - const prevCount = prevVisibleCountRef.current; - if (prevCount !== null && prevCount > 0 && visibleCount === 0) { - onBecameEmpty(); - } - prevVisibleCountRef.current = visibleCount; - }, [mode, filePath, sourceFilter, gitStatus, onBecameEmpty, visibleCount]); -} - -type FilterVisibleFilesOpts = { - mode: "all" | "file"; - filePath: string | undefined; - fileRepositoryName: string | undefined; - sourceFilter: "all" | ReviewSource; - rawPRFiles?: ReviewFile[]; -}; - -function filterVisibleFiles(allFiles: ReviewFile[], opts: FilterVisibleFilesOpts): ReviewFile[] { - const { mode, filePath, fileRepositoryName, sourceFilter, rawPRFiles } = opts; - // In file mode with an explicit PR source, bypass the deduplicated allFiles and - // read from rawPRFiles. This is necessary because allFiles deduplicates with - // priority uncommitted > committed > PR: a file that also has local changes - // will not appear with source "pr" in allFiles, causing "No changes" in PR rows. - if (mode === "file" && filePath && sourceFilter === "pr" && rawPRFiles && rawPRFiles.length > 0) { - let prFiles = rawPRFiles.filter((f) => f.path === filePath && f.source === "pr"); - if (fileRepositoryName !== undefined) { - prFiles = prFiles.filter((f) => (f.repository_name ?? "") === fileRepositoryName); - } - if (prFiles.length > 0) return prFiles; - } - let files = allFiles; - if (mode === "file" && filePath) { - files = files.filter((file) => file.path === filePath); - if (fileRepositoryName !== undefined) { - files = files.filter((file) => (file.repository_name ?? "") === fileRepositoryName); - } - } - if (sourceFilter !== "all") { - files = files.filter((file) => file.source === sourceFilter); - } - return files; -} - -function useVisibleDiffState(opts: { - allFiles: ReviewFile[]; - rawPRFiles: ReviewFile[]; - mode: "all" | "file"; - filePath: string | undefined; - fileRepositoryName: string | undefined; - sourceFilter: "all" | ReviewSource; - fileRefs: Map>; - reviewedFiles: Set; - staleFiles: Set; -}) { - const { - allFiles, - rawPRFiles, - mode, - filePath, - fileRepositoryName, - sourceFilter, - fileRefs, - reviewedFiles, - staleFiles, - } = opts; - const visibleFiles = useMemo( - () => - filterVisibleFiles(allFiles, { - mode, - filePath, - fileRepositoryName, - sourceFilter, - rawPRFiles, - }), - [allFiles, rawPRFiles, mode, filePath, fileRepositoryName, sourceFilter], - ); - const visibleFileRefs = useMemo(() => { - if (mode !== "file" || !filePath) return fileRefs; - const refs = new Map>(); - for (const [key, ref] of fileRefs.entries()) { - const split = splitReviewFileKey(key); - if (split.path !== filePath) continue; - if (fileRepositoryName !== undefined && (split.repositoryName || "") !== fileRepositoryName) { - continue; - } - refs.set(key, ref); - } - return refs; - }, [mode, filePath, fileRepositoryName, fileRefs]); - const reviewedCount = useMemo( - () => - visibleFiles.reduce((count, file) => { - const key = reviewFileKey(file); - if (!staleFiles.has(key) && reviewedFiles.has(key)) return count + 1; - return count; - }, 0), - [visibleFiles, reviewedFiles, staleFiles], - ); - const totalCount = visibleFiles.length; - const progressPercent = totalCount > 0 ? (reviewedCount / totalCount) * 100 : 0; - return { visibleFiles, visibleFileRefs, reviewedCount, totalCount, progressPercent }; -} - const TaskChangesPanel = memo(function TaskChangesPanel({ mode = "all", filePath, fileRepositoryName, + prKey, selectedDiff, onClearSelected, onBecameEmpty, @@ -458,50 +342,38 @@ const TaskChangesPanel = memo(function TaskChangesPanel({ const { openFile: panelOpenFile, openFileInMarkdownPreview } = usePanelActions(); const handleOpenFile = onOpenFileProp ?? panelOpenFile; - const { - activeSessionId, - allFiles, - rawPRFiles, - reviewedFiles, - staleFiles, - totalCommentCount, - fileRefs, - cumulativeLoading, - prDiffLoading, - gitStatus, - truncatedFilesCount, - } = useChangesView(selectedDiff, onClearSelected); - const { - splitView, - wordWrap, - setWordWrap, - autoMarkOnScroll, - handleToggleSplitView, - handleToggleReviewed, - handleDiscard, - handleToggleAutoMark, - handleFixComments, - } = useChangesActions(activeSessionId, allFiles, wordWrapProp); - const handleRequestWalkthrough = useWalkthroughRequest(activeSessionId, allFiles); - const { visibleFiles, visibleFileRefs, reviewedCount, totalCount, progressPercent } = - useVisibleDiffState({ - allFiles, - rawPRFiles, - mode, - filePath, - fileRepositoryName, - sourceFilter, - fileRefs, - reviewedFiles, - staleFiles, - }); - const selectedFileKey = useSelectedFileKey(mode, filePath, fileRepositoryName); + const view = useChangesView(selectedDiff, onClearSelected, sourceFilter, prKey); + const usesPRDiff = sourceFilter === "all" || sourceFilter === "pr"; + const relevantPRLoading = usesPRDiff && view.prDiffLoading; + const actions = useChangesActions(view.activeSessionId, view.allFiles, wordWrapProp); + const handleRequestWalkthrough = useWalkthroughRequest(view.activeSessionId, view.allFiles); + const visible = useVisibleDiffState({ + allFiles: view.allFiles, + rawPRFiles: view.rawPRFiles, + mode, + filePath, + fileRepositoryName, + sourceFilter, + prKey, + fileRefs: view.fileRefs, + reviewedFiles: view.reviewedFiles, + staleFiles: view.staleFiles, + }); + const { selectedFileKey, blockChangesForPR } = useChangesPRPresentation({ + sourceFilter, + prKey, + mode, + filePath, + fileRepositoryName, + visibleFiles: visible.visibleFiles, + }); useAutoCloseWhenEmpty({ mode, filePath, sourceFilter, - gitStatus, - visibleCount: visibleFiles.length, + gitStatus: view.gitStatus, + visibleCount: visible.visibleFiles.length, + prDiffLoading: relevantPRLoading, onBecameEmpty, }); @@ -510,37 +382,48 @@ const TaskChangesPanel = memo(function TaskChangesPanel({ return ( - - + + + + ); @@ -608,4 +491,4 @@ function ChangesPanelContent({ ); } -export { TaskChangesPanel, filterVisibleFiles, scrollToFileAndClear }; +export { TaskChangesPanel, scrollToFileAndClear }; diff --git a/apps/web/components/task/use-review-dialog.ts b/apps/web/components/task/use-review-dialog.ts index 0c2b537d24..896b959d07 100644 --- a/apps/web/components/task/use-review-dialog.ts +++ b/apps/web/components/task/use-review-dialog.ts @@ -8,16 +8,12 @@ import { } from "@/hooks/domains/session/use-session-git-status"; import { useCumulativeDiff } from "@/hooks/domains/session/use-cumulative-diff"; import { useFileEditors } from "@/hooks/use-file-editors"; -import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr"; +import { useReviewPRSelection } from "@/hooks/domains/github/use-review-pr-selection"; import { usePRDiff } from "@/hooks/domains/github/use-pr-diff"; +import { usePRReviewRepositoryIdentity } from "@/hooks/domains/github/use-pr-review-repository-identity"; import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories"; -import { useRepository } from "@/hooks/domains/workspace/use-repository"; import { formatReviewCommentsAsMarkdown } from "@/components/task/chat/messages/review-comments-attachment"; -import { - getCumulativeReviewRepositoryNames, - isReviewMultiRepo, - resolvePRReviewRepositoryName, -} from "@/components/review/types"; +import { getCumulativeReviewRepositoryNames, isReviewMultiRepo } from "@/components/review/types"; import { getWebSocketClient } from "@/lib/ws/connection"; import { useToast } from "@/components/toast-provider"; import type { DiffComment } from "@/lib/diff/types"; @@ -111,16 +107,22 @@ export function useReviewDialog(effectiveSessionId: string | null) { cumulativeRepositoryNames, ); const { openFile: reviewOpenFile } = useFileEditors(); - const reviewTaskPR = useActiveTaskPR(); - const reviewPRRepository = useRepository(reviewTaskPR?.repository_id ?? null); - const reviewPRRepositoryName = resolvePRReviewRepositoryName( + const { + prs: reviewPRs, + selectedPR: reviewTaskPR, + selectedKey: reviewSelectedPRKey, + selectPR: reviewSelectPR, + } = useReviewPRSelection(activeTaskId); + const reviewPRRepositoryName = usePRReviewRepositoryIdentity( + activeTaskId, + effectiveSessionId, reviewTaskPR, - reviewPRRepository?.name, ); - const { files: reviewPRDiffFiles } = usePRDiff( + const reviewPRDiff = usePRDiff( reviewTaskPR?.owner ?? null, reviewTaskPR?.repo ?? null, reviewTaskPR?.pr_number ?? null, + reviewTaskPR?.last_synced_at ?? null, ); const handleReviewSendComments = useCallback( @@ -155,7 +157,14 @@ export function useReviewDialog(effectiveSessionId: string | null) { baseBranch, reviewGitStatusFiles: reviewGitStatus.files, reviewCumulativeDiff, - reviewPRDiffFiles, + reviewPRs, + reviewSelectedPR: reviewTaskPR, + reviewSelectedPRKey, + reviewSelectPR, + reviewPRDiffFiles: reviewPRDiff.files, + reviewPRDiffLoading: reviewPRDiff.loading, + reviewPRDiffError: reviewPRDiff.error, + reviewRefreshPRDiff: reviewPRDiff.refresh, reviewPRRepoName: reviewGitStatus.isMultiRepo ? reviewPRRepositoryName : undefined, reviewUseRepositoryKeys: reviewGitStatus.isMultiRepo, reviewOpenFile, diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 01deb7e7b2..28d3a2affe 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -1123,6 +1123,14 @@ export class ApiClient { await this.request("POST", "/api/v1/github/mock/task-prs", data); } + async associateGitHubTaskPR(data: { + task_id: string; + repository_id: string; + pr_url: string; + }): Promise { + await this.request("POST", "/api/v1/github/task-prs", data); + } + async getTaskCIAutomationOptions(taskId: string): Promise { return this.request("GET", `/api/v1/github/tasks/${encodeURIComponent(taskId)}/ci-options`); } diff --git a/apps/web/e2e/helpers/multi-pr-review.ts b/apps/web/e2e/helpers/multi-pr-review.ts new file mode 100644 index 0000000000..babd2e5d0f --- /dev/null +++ b/apps/web/e2e/helpers/multi-pr-review.ts @@ -0,0 +1,85 @@ +import type { SeedData } from "../fixtures/test-base"; +import type { ApiClient } from "./api-client"; + +export const REVIEW_OWNER = "testorg"; +export const REVIEW_REPO = "testrepo"; +export const REVIEW_SHARED_FILE = "shared-pr.ts"; + +export const REVIEW_PRS = [ + { + number: 121, + title: "First review branch", + branch: "feat/first-review", + marker: "FIRST_PR_MARKER", + repositoryName: "E2E-Repo", + }, + { + number: 122, + title: "Second review branch", + branch: "feat/second-review", + marker: "SECOND_PR_MARKER", + repositoryName: "E2E-Repo-feat-second-review", + }, +] as const; + +export async function seedMultiPRReviewTask( + apiClient: ApiClient, + seedData: SeedData, + title: string, + description = "/e2e:simple-message", +) { + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("reviewer"); + + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description, + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repositories: REVIEW_PRS.map((pr) => ({ + repository_id: seedData.repositoryId, + base_branch: "main", + checkout_branch: pr.branch, + })), + executor_profile_id: seedData.worktreeExecutorProfileId, + }, + ); + + await apiClient.mockGitHubAddPRs( + REVIEW_PRS.map((pr) => ({ + number: pr.number, + title: pr.title, + state: "open", + head_branch: pr.branch, + base_branch: "main", + author_login: "reviewer", + repo_owner: REVIEW_OWNER, + repo_name: REVIEW_REPO, + html_url: `https://github.com/${REVIEW_OWNER}/${REVIEW_REPO}/pull/${pr.number}`, + additions: 1, + deletions: 0, + })), + ); + + for (const pr of REVIEW_PRS) { + await apiClient.mockGitHubAddPRFiles(REVIEW_OWNER, REVIEW_REPO, pr.number, [ + { + filename: REVIEW_SHARED_FILE, + status: "added", + additions: 1, + deletions: 0, + patch: `@@ -0,0 +1 @@\n+${pr.marker}`, + }, + ]); + await apiClient.associateGitHubTaskPR({ + task_id: task.id, + repository_id: seedData.repositoryId, + pr_url: `https://github.com/${REVIEW_OWNER}/${REVIEW_REPO}/pull/${pr.number}`, + }); + } + + return task; +} diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index b623d198df..4a99ac90fd 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -840,6 +840,59 @@ export class SessionPage { return this.page.getByTestId("review-request-walkthrough"); } + /** Expanded Review dialog shared by the desktop and mobile task layouts. */ + reviewDialog(): Locator { + return this.page.getByRole("dialog", { name: "Review Changes" }); + } + + /** Current-PR trigger rendered in Review when the task has multiple PRs. */ + reviewPRSelectorTrigger(): Locator { + return this.page.getByTestId("review-pr-selector-trigger"); + } + + /** Portaled PR selector menu; intentionally page-scoped rather than dialog-scoped. */ + reviewPRSelectorMenu(): Locator { + return this.page.getByTestId("review-pr-selector-menu"); + } + + /** One PR choice in the expanded Review selector. */ + reviewPRSelectorItem(owner: string, repo: string, prNumber: number): Locator { + return this.page.getByTestId(`review-pr-selector-item-${owner}-${repo}-${prNumber}`); + } + + /** Sticky diff header for one file in the expanded Review dialog. */ + reviewFileHeader(path: string): Locator { + return this.reviewDialog().locator( + `[data-testid="review-file-header"][data-file-path=${JSON.stringify(path)}]`, + ); + } + + /** + * Read visible Review diff text from @pierre/diffs shadow roots. + * + * Dockview can leave hidden diff surfaces mounted, so scope to the active + * Review dialog and ignore zero-size/hidden containers before reading them. + */ + async reviewDiffText(): Promise { + return this.reviewDialog().evaluate((dialog) => { + const visibleContainers = Array.from(dialog.querySelectorAll("diffs-container")).filter( + (container) => { + const bounds = container.getBoundingClientRect(); + const style = window.getComputedStyle(container); + return ( + bounds.width > 0 && + bounds.height > 0 && + style.display !== "none" && + style.visibility !== "hidden" + ); + }, + ); + return visibleContainers + .map((container) => container.shadowRoot?.textContent ?? "") + .join("\n"); + }); + } + walkthroughLauncher(): Locator { return this.page.getByTestId("walkthrough-launcher"); } diff --git a/apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts b/apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts new file mode 100644 index 0000000000..0c32d4386d --- /dev/null +++ b/apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts @@ -0,0 +1,140 @@ +import { test, expect } from "../../fixtures/test-base"; +import { + REVIEW_OWNER, + REVIEW_PRS, + REVIEW_REPO, + REVIEW_SHARED_FILE, + seedMultiPRReviewTask, +} from "../../helpers/multi-pr-review"; +import { SessionPage } from "../../pages/session-page"; +import type { Page } from "@playwright/test"; + +async function openMobileReview(testPage: Page, session: SessionPage) { + await testPage.getByRole("button", { name: "Changes" }).tap(); + const changesPanel = testPage.getByTestId("mobile-changes-panel"); + await expect(changesPanel).toBeVisible({ timeout: 15_000 }); + const prFiles = changesPanel.getByTestId("pr-files-section"); + await expect(prFiles).toBeVisible({ timeout: 20_000 }); + for (const pr of REVIEW_PRS) { + await expect( + prFiles.locator( + `[data-changes-file=${JSON.stringify(REVIEW_SHARED_FILE)}][data-pr-key="${REVIEW_OWNER}/${REVIEW_REPO}/${pr.number}"]`, + ), + ).toBeVisible(); + } + await changesPanel.getByRole("button", { name: "Review", exact: true }).tap(); + await expect(session.reviewDialog()).toBeVisible({ timeout: 15_000 }); +} + +test.describe("Review dialog multi-PR selector on mobile", () => { + test.describe.configure({ timeout: 120_000 }); + + test("switches PRs from a contained touch menu without viewport overflow", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await seedMultiPRReviewTask(apiClient, seedData, "Mobile Multi-PR Review E2E"); + await testPage.goto(`/t/${task.id}`); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle(); + await openMobileReview(testPage, session); + + const [firstPR, secondPR] = REVIEW_PRS; + const selector = session.reviewPRSelectorTrigger(); + await expect(selector).toBeVisible(); + await expect(selector).toHaveAttribute("data-pr-number", String(firstPR.number)); + await expect(session.reviewFileHeader(REVIEW_SHARED_FILE)).toBeVisible(); + await expect(session.reviewDialog().getByTestId("changes-repo-group")).toHaveAttribute( + "data-repository-name", + firstPR.repositoryName, + ); + await expect + .poll(() => session.reviewDiffText(), { timeout: 30_000 }) + .toContain(firstPR.marker); + + await selector.tap(); + const menu = session.reviewPRSelectorMenu(); + const secondItem = session.reviewPRSelectorItem(REVIEW_OWNER, REVIEW_REPO, secondPR.number); + await expect(menu).toBeVisible(); + await expect(secondItem).toBeVisible(); + + const [selectorBox, menuBox, itemBox, viewport] = await Promise.all([ + selector.boundingBox(), + menu.boundingBox(), + secondItem.boundingBox(), + testPage.evaluate(() => ({ width: window.innerWidth, height: window.innerHeight })), + ]); + if (!selectorBox || !menuBox || !itemBox) { + throw new Error("Review PR selector geometry is unavailable"); + } + expect(selectorBox.height).toBeGreaterThanOrEqual(44); + expect(itemBox.height).toBeGreaterThanOrEqual(44); + expect(menuBox.x).toBeGreaterThanOrEqual(0); + expect(menuBox.y).toBeGreaterThanOrEqual(0); + expect(menuBox.x + menuBox.width).toBeLessThanOrEqual(viewport.width + 1); + expect(menuBox.y + menuBox.height).toBeLessThanOrEqual(viewport.height + 1); + + const openMenuOverflow = await testPage.evaluate(() => ({ + viewport: document.documentElement.clientWidth, + document: document.documentElement.scrollWidth, + })); + expect(openMenuOverflow.document).toBeLessThanOrEqual(openMenuOverflow.viewport + 1); + + await secondItem.tap(); + await expect(menu).toBeHidden(); + await expect(session.reviewDialog()).toBeVisible(); + await expect(selector).toHaveAttribute("data-pr-number", String(secondPR.number)); + await expect(session.reviewFileHeader(REVIEW_SHARED_FILE)).toBeVisible({ timeout: 20_000 }); + await expect(session.reviewDialog().getByTestId("changes-repo-group")).toHaveAttribute( + "data-repository-name", + secondPR.repositoryName, + ); + await expect + .poll(() => session.reviewDiffText(), { timeout: 30_000 }) + .toContain(secondPR.marker); + await expect.poll(() => session.reviewDiffText()).not.toContain(firstPR.marker); + + const switchedOverflow = await testPage.evaluate(() => ({ + viewport: document.documentElement.clientWidth, + document: document.documentElement.scrollWidth, + })); + expect(switchedOverflow.document).toBeLessThanOrEqual(switchedOverflow.viewport + 1); + }); + + test("keeps the selector usable in the coarse-pointer tablet layout", async ({ + testPage, + apiClient, + seedData, + }) => { + await testPage.setViewportSize({ width: 820, height: 900 }); + const task = await seedMultiPRReviewTask(apiClient, seedData, "Tablet Multi-PR Review E2E"); + await testPage.goto(`/t/${task.id}`); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle(); + await expect(testPage.getByTestId("tablet-task-layout")).toBeVisible(); + + await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog"))); + await expect(session.reviewDialog()).toBeVisible(); + const dialogSelector = session.reviewPRSelectorTrigger(); + await expect(dialogSelector).toBeVisible(); + const [dialogSelectorBox, toolbarBox] = await Promise.all([ + dialogSelector.boundingBox(), + dialogSelector.locator("xpath=../..").boundingBox(), + ]); + if (!dialogSelectorBox || !toolbarBox) { + throw new Error("Tablet Review selector geometry is unavailable"); + } + expect(Math.round(dialogSelectorBox.height)).toBeGreaterThanOrEqual(44); + expect(toolbarBox.height).toBeGreaterThanOrEqual(44); + + await dialogSelector.tap(); + await session.reviewPRSelectorItem(REVIEW_OWNER, REVIEW_REPO, REVIEW_PRS[1].number).tap(); + await expect(dialogSelector).toHaveAttribute("data-pr-number", String(REVIEW_PRS[1].number)); + await expect.poll(() => session.reviewDiffText()).toContain(REVIEW_PRS[1].marker); + }); +}); diff --git a/apps/web/e2e/tests/review/review-multi-pr.spec.ts b/apps/web/e2e/tests/review/review-multi-pr.spec.ts new file mode 100644 index 0000000000..a810ba92de --- /dev/null +++ b/apps/web/e2e/tests/review/review-multi-pr.spec.ts @@ -0,0 +1,73 @@ +import { test, expect } from "../../fixtures/test-base"; +import { + REVIEW_OWNER, + REVIEW_PRS, + REVIEW_REPO, + REVIEW_SHARED_FILE, + seedMultiPRReviewTask, +} from "../../helpers/multi-pr-review"; +import { SessionPage } from "../../pages/session-page"; + +async function openDesktopReview(session: SessionPage) { + await session.clickTab("Changes"); + const prFiles = session.prFilesSection(); + await expect(prFiles).toBeVisible({ timeout: 20_000 }); + for (const pr of REVIEW_PRS) { + await expect( + prFiles.locator( + `[data-changes-file=${JSON.stringify(REVIEW_SHARED_FILE)}][data-pr-key="${REVIEW_OWNER}/${REVIEW_REPO}/${pr.number}"]`, + ), + ).toBeVisible(); + } + await session.changes.getByRole("button", { name: "Review", exact: true }).click(); + await expect(session.reviewDialog()).toBeVisible({ timeout: 15_000 }); +} + +test.describe("Review dialog multi-PR selector", () => { + test.describe.configure({ timeout: 120_000 }); + + test("switches between same-repository PRs without stale files or diff content", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await seedMultiPRReviewTask(apiClient, seedData, "Multi-PR Review E2E"); + await testPage.goto(`/t/${task.id}`); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle(); + await openDesktopReview(session); + + const [firstPR, secondPR] = REVIEW_PRS; + const selector = session.reviewPRSelectorTrigger(); + await expect(selector).toBeVisible(); + await expect(selector).toHaveAttribute("data-pr-number", String(firstPR.number)); + await expect(session.reviewFileHeader(REVIEW_SHARED_FILE)).toBeVisible(); + await expect(session.reviewDialog().getByTestId("changes-repo-group")).toHaveAttribute( + "data-repository-name", + firstPR.repositoryName, + ); + await expect + .poll(() => session.reviewDiffText(), { timeout: 30_000 }) + .toContain(firstPR.marker); + + await selector.click(); + const menu = session.reviewPRSelectorMenu(); + await expect(menu).toBeVisible(); + await session.reviewPRSelectorItem(REVIEW_OWNER, REVIEW_REPO, secondPR.number).click(); + + await expect(menu).toBeHidden(); + await expect(session.reviewDialog()).toBeVisible(); + await expect(selector).toHaveAttribute("data-pr-number", String(secondPR.number)); + await expect(session.reviewFileHeader(REVIEW_SHARED_FILE)).toBeVisible({ timeout: 20_000 }); + await expect(session.reviewDialog().getByTestId("changes-repo-group")).toHaveAttribute( + "data-repository-name", + secondPR.repositoryName, + ); + await expect + .poll(() => session.reviewDiffText(), { timeout: 30_000 }) + .toContain(secondPR.marker); + await expect.poll(() => session.reviewDiffText()).not.toContain(firstPR.marker); + }); +}); diff --git a/apps/web/hooks/domains/github/use-pr-diff.test.ts b/apps/web/hooks/domains/github/use-pr-diff.test.ts new file mode 100644 index 0000000000..ad769a168d --- /dev/null +++ b/apps/web/hooks/domains/github/use-pr-diff.test.ts @@ -0,0 +1,100 @@ +import { act, renderHook, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { resolvePRDiffView, usePRDiff, type KeyedPRDiffState } from "./use-pr-diff"; + +const requestMock = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => ({ request: requestMock }), +})); + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +beforeEach(() => { + requestMock.mockReset(); +}); + +const staleState: KeyedPRDiffState = { + sourceKey: "acme/app/1/old", + files: [ + { + filename: "src/first-pr.ts", + status: "modified", + patch: "@@ -1 +1 @@", + additions: 1, + deletions: 1, + }, + ], + loading: false, + error: null, +}; + +describe("resolvePRDiffView", () => { + it("masks the previous PR while a newly requested source starts loading", () => { + expect(resolvePRDiffView(staleState, "acme/app/2/new")).toEqual({ + files: [], + loading: true, + error: null, + }); + }); + + it("clears stale files without loading when no PR is requested", () => { + expect(resolvePRDiffView(staleState, "")).toEqual({ + files: [], + loading: false, + error: null, + }); + }); +}); + +describe("usePRDiff request ownership", () => { + it("masks A immediately on B and ignores A when its request resolves late", async () => { + const first = deferred<{ files: KeyedPRDiffState["files"] }>(); + const second = deferred<{ files: KeyedPRDiffState["files"] }>(); + requestMock.mockImplementation((_action: string, payload: { number: number }) => + payload.number === 1 ? first.promise : second.promise, + ); + const { result, rerender } = renderHook( + ({ number }) => usePRDiff("acme", "app", number, "synced"), + { initialProps: { number: 1 } }, + ); + + rerender({ number: 2 }); + expect(result.current).toMatchObject({ files: [], loading: true, error: null }); + + second.resolve({ + files: [ + { + filename: "src/second-pr.ts", + status: "modified", + patch: "@@second@@", + additions: 1, + deletions: 0, + }, + ], + }); + await waitFor(() => expect(result.current.files[0]?.filename).toBe("src/second-pr.ts")); + + await act(async () => { + first.resolve({ + files: [ + { + filename: "src/first-pr.ts", + status: "modified", + patch: "@@first@@", + additions: 1, + deletions: 0, + }, + ], + }); + await first.promise; + }); + + expect(result.current.files[0]?.filename).toBe("src/second-pr.ts"); + }); +}); diff --git a/apps/web/hooks/domains/github/use-pr-diff.ts b/apps/web/hooks/domains/github/use-pr-diff.ts index 88db31790d..3beafa6c96 100644 --- a/apps/web/hooks/domains/github/use-pr-diff.ts +++ b/apps/web/hooks/domains/github/use-pr-diff.ts @@ -7,28 +7,43 @@ import type { PRDiffFile } from "@/lib/types/github"; const debug = createDebugLogger("review:pr-diff"); -type PRDiffState = { +type PRDiffView = { files: PRDiffFile[]; loading: boolean; error: string | null; }; -const INITIAL_STATE: PRDiffState = { +export type KeyedPRDiffState = PRDiffView & { + sourceKey: string; +}; + +const INITIAL_STATE: KeyedPRDiffState = { + sourceKey: "", files: [], loading: false, error: null, }; +export function resolvePRDiffView(state: KeyedPRDiffState, requestedKey: string): PRDiffView { + if (state.sourceKey === requestedKey) { + return { files: state.files, loading: state.loading, error: state.error }; + } + return { files: [], loading: requestedKey !== "", error: null }; +} + async function fetchPRFiles( owner: string, repo: string, prNumber: number, - setState: (s: PRDiffState) => void, + sourceKey: string, + setState: (s: KeyedPRDiffState) => void, ) { const client = getWebSocketClient(); - if (!client) return; - - setState({ files: [], loading: true, error: null }); + setState({ sourceKey, files: [], loading: true, error: null }); + if (!client) { + setState({ sourceKey, files: [], loading: false, error: null }); + return; + } debug("fetch.start", { owner, repo, prNumber }); try { const response = await client.request<{ files?: PRDiffFile[] }>("github.pr_files.get", { @@ -37,11 +52,11 @@ async function fetchPRFiles( number: prNumber, }); const files = response?.files ?? []; - setState({ files, loading: false, error: null }); + setState({ sourceKey, files, loading: false, error: null }); debug("fetch.success", { owner, repo, prNumber, fileCount: files.length }); } catch (err) { const message = err instanceof Error ? err.message : "Failed to fetch PR files"; - setState({ files: [], loading: false, error: message }); + setState({ sourceKey, files: [], loading: false, error: message }); debug("fetch.error", { owner, repo, prNumber, error: message }); } } @@ -56,36 +71,34 @@ export function usePRDiff( prNumber: number | null, refreshKey?: string | null, ) { - const [state, setState] = useState(INITIAL_STATE); + const [state, setState] = useState(INITIAL_STATE); const hasParams = !!owner && !!repo && !!prNumber; + const sourceKey = hasParams ? `${owner}/${repo}/${prNumber}/${refreshKey ?? ""}` : ""; const paramsKeyRef = useRef(""); const requestIdRef = useRef(0); const refresh = useCallback(() => { if (!owner || !repo || !prNumber) return; const requestId = ++requestIdRef.current; - void fetchPRFiles(owner, repo, prNumber, (next) => { + void fetchPRFiles(owner, repo, prNumber, sourceKey, (next) => { if (requestId !== requestIdRef.current) return; setState(next); }); - }, [owner, repo, prNumber]); + }, [owner, repo, prNumber, sourceKey]); useEffect(() => { - const key = hasParams ? `${owner}/${repo}/${prNumber}/${refreshKey ?? ""}` : ""; - if (key === paramsKeyRef.current) return; - paramsKeyRef.current = key; + if (sourceKey === paramsKeyRef.current) return; + paramsKeyRef.current = sourceKey; if (!owner || !repo || !prNumber) { requestIdRef.current++; // invalidate in-flight responses return; } const requestId = ++requestIdRef.current; - void fetchPRFiles(owner, repo, prNumber, (next) => { + void fetchPRFiles(owner, repo, prNumber, sourceKey, (next) => { if (requestId !== requestIdRef.current) return; setState(next); }); - }, [owner, repo, prNumber, hasParams, refreshKey]); + }, [owner, repo, prNumber, sourceKey]); - // Return initial state when params are null to clear stale data - if (!hasParams) return { ...INITIAL_STATE, refresh }; - return { ...state, refresh }; + return { ...resolvePRDiffView(state, sourceKey), refresh }; } diff --git a/apps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsx b/apps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsx new file mode 100644 index 0000000000..b36a76ece7 --- /dev/null +++ b/apps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsx @@ -0,0 +1,230 @@ +import { createElement, type ReactNode } from "react"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, it } from "vitest"; +import { StateProvider, useAppStore } from "@/components/state-provider"; +import type { AppState } from "@/lib/state/store"; +import type { TaskPR } from "@/lib/types/github"; +import { + repositoryId, + sessionId, + workspaceId, + type Repository, + type TaskSession, +} from "@/lib/types/http"; +import { usePRReviewRepositoryIdentity } from "./use-pr-review-repository-identity"; + +afterEach(cleanup); + +const TASK_ID = "task-1"; +const SESSION_ID = sessionId("session-1"); +const REPOSITORY_ID = repositoryId("repo-widgets"); +const WORKSPACE_ID = workspaceId("workspace-1"); +const REPOSITORY_NAME = "widgets"; +const SIBLING_BRANCH = "feat/second"; +const NOW = "2026-07-22T00:00:00Z"; +const selectedPR: TaskPR = { + id: "pr-2", + task_id: TASK_ID, + repository_id: REPOSITORY_ID, + owner: "acme", + repo: REPOSITORY_NAME, + pr_number: 42, + pr_url: "https://github.com/acme/widgets/pull/42", + pr_title: "Sibling branch", + head_branch: SIBLING_BRANCH, + base_branch: "main", + author_login: "reviewer", + state: "open", + review_state: "", + checks_state: "", + mergeable_state: "", + review_count: 0, + pending_review_count: 0, + comment_count: 0, + unresolved_review_threads: 0, + checks_total: 0, + checks_passing: 0, + additions: 1, + deletions: 0, + created_at: NOW, + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: NOW, +}; + +const repository: Repository = { + id: REPOSITORY_ID, + workspace_id: WORKSPACE_ID, + name: REPOSITORY_NAME, + source_type: "local", + local_path: "/repos/widgets", + provider: "github", + provider_repo_id: REPOSITORY_NAME, + provider_owner: "acme", + provider_name: REPOSITORY_NAME, + default_branch: "main", + worktree_branch_prefix: "", + pull_before_worktree: false, + setup_script: "", + cleanup_script: "", + dev_script: "", + copy_files: "", + created_at: NOW, + updated_at: NOW, +}; + +const initialState: Partial = { + kanban: { + workflowId: null, + steps: [], + tasks: [ + { + id: TASK_ID, + workflowStepId: "step-1", + title: "Multi-PR task", + position: 0, + repositories: [ + { + id: "task-repo-1", + repository_id: REPOSITORY_ID, + base_branch: "main", + checkout_branch: "feat/first", + position: 0, + }, + { + id: "task-repo-2", + repository_id: REPOSITORY_ID, + base_branch: "main", + checkout_branch: SIBLING_BRANCH, + position: 1, + }, + ], + }, + ], + }, + repositories: { + itemsByWorkspaceId: { [WORKSPACE_ID]: [repository] }, + loadingByWorkspaceId: {}, + loadedByWorkspaceId: { [WORKSPACE_ID]: true }, + }, + taskSessions: { + items: { + [SESSION_ID]: { + id: SESSION_ID, + task_id: TASK_ID, + state: "RUNNING", + worktrees: [ + { + id: "association-2", + session_id: SESSION_ID, + worktree_id: "worktree-2", + repository_id: REPOSITORY_ID, + worktree_branch: SIBLING_BRANCH, + worktree_path: "/tasks/example/widgets-feat-second", + position: 1, + }, + ], + } as TaskSession, + }, + }, +}; + +function wrapper({ children }: { children: ReactNode }) { + return createElement(StateProvider, { initialState, children }); +} + +describe("usePRReviewRepositoryIdentity", () => { + it("normalizes boot session worktrees to the selected branch directory", () => { + const { result } = renderHook( + () => usePRReviewRepositoryIdentity(TASK_ID, SESSION_ID, selectedPR), + { wrapper }, + ); + + expect(result.current).toBe("widgets-feat-second"); + }); + + it("uses a live worktree when boot metadata has not arrived", () => { + const { result } = renderHook( + () => { + const setWorktree = useAppStore((state) => state.setWorktree); + const setSessionWorktrees = useAppStore((state) => state.setSessionWorktrees); + return { + identity: usePRReviewRepositoryIdentity(TASK_ID, SESSION_ID, selectedPR), + setWorktree, + setSessionWorktrees, + }; + }, + { + wrapper: ({ children }) => + createElement(StateProvider, { + initialState: { + ...initialState, + taskSessions: { items: {} }, + }, + children, + }), + }, + ); + + act(() => { + result.current.setWorktree({ + id: "live-worktree-2", + sessionId: SESSION_ID, + repositoryId: REPOSITORY_ID, + branch: SIBLING_BRANCH, + path: "/tasks/example/widgets-live-second", + }); + result.current.setSessionWorktrees(SESSION_ID, ["live-worktree-2"]); + }); + + expect(result.current.identity).toBe("widgets-live-second"); + }); + + it("fills partial boot metadata from the same live worktree ID", () => { + const partialSession = { + ...(initialState.taskSessions?.items[SESSION_ID] as TaskSession), + worktrees: [ + { + id: "association-2", + session_id: SESSION_ID, + worktree_id: "worktree-2", + worktree_branch: "", + worktree_path: "", + position: 1, + }, + ], + } as TaskSession; + const { result } = renderHook( + () => { + const setWorktree = useAppStore((state) => state.setWorktree); + return { + identity: usePRReviewRepositoryIdentity(TASK_ID, SESSION_ID, selectedPR), + setWorktree, + }; + }, + { + wrapper: ({ children }) => + createElement(StateProvider, { + initialState: { + ...initialState, + taskSessions: { items: { [SESSION_ID]: partialSession } }, + }, + children, + }), + }, + ); + + act(() => { + result.current.setWorktree({ + id: "worktree-2", + sessionId: SESSION_ID, + repositoryId: REPOSITORY_ID, + branch: SIBLING_BRANCH, + path: "/tasks/example/widgets-live-hydrated", + }); + }); + + expect(result.current.identity).toBe("widgets-live-hydrated"); + }); +}); diff --git a/apps/web/hooks/domains/github/use-pr-review-repository-identity.ts b/apps/web/hooks/domains/github/use-pr-review-repository-identity.ts new file mode 100644 index 0000000000..15064da363 --- /dev/null +++ b/apps/web/hooks/domains/github/use-pr-review-repository-identity.ts @@ -0,0 +1,80 @@ +"use client"; + +import { useMemo } from "react"; +import { useAppStore } from "@/components/state-provider"; +import { + resolvePRReviewRepositoryIdentity, + type ReviewWorktreeIdentity, +} from "@/components/review/review-repository-identity"; +import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories"; +import { useRepository } from "@/hooks/domains/workspace/use-repository"; +import type { Worktree } from "@/lib/state/slices/session/types"; +import type { TaskPR } from "@/lib/types/github"; +import type { TaskSessionWorktree } from "@/lib/types/http"; + +const EMPTY_WORKTREE_IDS: string[] = []; + +function normalizeReviewWorktrees( + sessionWorktrees: TaskSessionWorktree[] | undefined, + liveWorktreeIds: string[], + liveWorktrees: Record, +): ReviewWorktreeIdentity[] { + const normalized: ReviewWorktreeIdentity[] = (sessionWorktrees ?? []).map((worktree) => { + const id = worktree.worktree_id || worktree.id; + const live = liveWorktrees[id]; + return { + id, + repositoryId: worktree.repository_id || live?.repositoryId, + branchSlug: worktree.branch_slug, + branch: worktree.worktree_branch || live?.branch, + path: worktree.worktree_path || live?.path, + position: worktree.position, + }; + }); + const seen = new Set(normalized.map((worktree) => worktree.id)); + for (const id of liveWorktreeIds) { + const worktree = liveWorktrees[id]; + if (!worktree || seen.has(id)) continue; + normalized.push({ + id: worktree.id, + repositoryId: worktree.repositoryId, + branch: worktree.branch, + path: worktree.path, + }); + } + return normalized; +} + +export function usePRReviewRepositoryIdentity( + taskId: string | null, + sessionId: string | null | undefined, + pr: TaskPR | null, +): string | undefined { + const taskRepositories = useTaskRepositories(taskId); + const workspaceRepository = useRepository(pr?.repository_id ?? null); + const sessionWorktrees = useAppStore((state) => + sessionId ? state.taskSessions.items[sessionId]?.worktrees : undefined, + ); + const liveWorktreeIds = useAppStore((state) => + sessionId + ? (state.sessionWorktreesBySessionId.itemsBySessionId[sessionId] ?? EMPTY_WORKTREE_IDS) + : EMPTY_WORKTREE_IDS, + ); + const liveWorktrees = useAppStore((state) => state.worktrees.items); + + const worktrees = useMemo( + () => normalizeReviewWorktrees(sessionWorktrees, liveWorktreeIds, liveWorktrees), + [sessionWorktrees, liveWorktreeIds, liveWorktrees], + ); + + return useMemo( + () => + resolvePRReviewRepositoryIdentity({ + pr, + workspaceRepositoryName: workspaceRepository?.name, + taskRepositories, + worktrees, + }), + [pr, workspaceRepository?.name, taskRepositories, worktrees], + ); +} diff --git a/apps/web/hooks/domains/github/use-review-pr-selection.test.ts b/apps/web/hooks/domains/github/use-review-pr-selection.test.ts new file mode 100644 index 0000000000..f68ed5a529 --- /dev/null +++ b/apps/web/hooks/domains/github/use-review-pr-selection.test.ts @@ -0,0 +1,120 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createElement, type ReactNode } from "react"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { StateProvider, useAppStore } from "@/components/state-provider"; +import type { TaskPR } from "@/lib/types/github"; +import { + findReviewPRByKey, + resolveReviewPRSelection, + useReviewPRSelection, +} from "./use-review-pr-selection"; + +const requestMock = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => ({ request: requestMock }), +})); + +afterEach(() => cleanup()); +beforeEach(() => { + requestMock.mockReset().mockResolvedValue({ prs: [] }); +}); + +function wrapper({ children }: { children: ReactNode }) { + return createElement(StateProvider, null, children); +} + +function makeTaskPR(overrides: Partial = {}): TaskPR { + return { + id: "pr-1", + task_id: "task-1", + owner: "acme", + repo: "widget", + pr_number: 1, + pr_url: "https://github.com/acme/widget/pull/1", + pr_title: "First branch", + head_branch: "feat/first", + base_branch: "main", + author_login: "alice", + state: "open", + review_state: "", + checks_state: "", + mergeable_state: "", + review_count: 0, + pending_review_count: 0, + comment_count: 0, + unresolved_review_threads: 0, + checks_total: 0, + checks_passing: 0, + additions: 1, + deletions: 0, + created_at: "2026-07-01T00:00:00Z", + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: "2026-07-01T00:00:00Z", + ...overrides, + }; +} + +describe("resolveReviewPRSelection", () => { + it("defaults to the primary PR when no override exists", () => { + const primary = makeTaskPR(); + + expect(resolveReviewPRSelection([primary])).toEqual(primary); + }); + + it("selects an explicit sibling PR instead of always returning the primary", () => { + const primary = makeTaskPR(); + const sibling = makeTaskPR({ + id: "pr-2", + pr_number: 2, + pr_url: "https://github.com/acme/widget/pull/2", + pr_title: "Second branch", + head_branch: "feat/second", + }); + + expect(resolveReviewPRSelection([primary, sibling], "acme/widget/2")).toEqual(sibling); + }); + + it("falls back to the primary PR when the selected sibling disappears", () => { + const primary = makeTaskPR(); + + expect(resolveReviewPRSelection([primary], "acme/widget/2")).toEqual(primary); + }); +}); + +describe("findReviewPRByKey", () => { + it("does not substitute the primary PR when an exact panel PR disappears", () => { + const primary = makeTaskPR(); + + expect(findReviewPRByKey([primary], "acme/widget/2")).toBeNull(); + }); +}); + +describe("useReviewPRSelection", () => { + it("reads task PRs without starting another task-level sync loop", () => { + renderHook(() => useReviewPRSelection("task-1"), { wrapper }); + + expect(requestMock).not.toHaveBeenCalled(); + }); + + it("updates the selected PR for the current task", () => { + const primary = makeTaskPR(); + const sibling = makeTaskPR({ id: "pr-2", pr_number: 2, head_branch: "feat/second" }); + const { result } = renderHook( + () => { + const setTaskPRs = useAppStore((state) => state.setTaskPRs); + return { + setTaskPRs, + selection: useReviewPRSelection("task-1"), + }; + }, + { wrapper }, + ); + + act(() => result.current.setTaskPRs({ "task-1": [primary, sibling] })); + act(() => result.current.selection.selectPR(sibling)); + + expect(result.current.selection.selectedPR).toEqual(sibling); + }); +}); diff --git a/apps/web/hooks/domains/github/use-review-pr-selection.ts b/apps/web/hooks/domains/github/use-review-pr-selection.ts new file mode 100644 index 0000000000..6f29f8fd31 --- /dev/null +++ b/apps/web/hooks/domains/github/use-review-pr-selection.ts @@ -0,0 +1,38 @@ +"use client"; + +import { useCallback } from "react"; +import { useAppStore } from "@/components/state-provider"; +import { prTaskKey } from "@/components/github/pr-utils"; +import type { TaskPR } from "@/lib/types/github"; +import { getPrimaryTaskPR } from "./use-task-pr"; + +const EMPTY_PRS: TaskPR[] = []; + +export function findReviewPRByKey(prs: TaskPR[], selectedKey: string): TaskPR | null { + return prs.find((pr) => prTaskKey(pr) === selectedKey) ?? null; +} + +export function resolveReviewPRSelection(prs: TaskPR[], selectedKey?: string): TaskPR | null { + if (!selectedKey) return getPrimaryTaskPR(prs); + return findReviewPRByKey(prs, selectedKey) ?? getPrimaryTaskPR(prs); +} + +export function useReviewPRSelection(taskId: string | null) { + const prs = useAppStore((state) => + taskId ? (state.taskPRs.byTaskId[taskId] ?? EMPTY_PRS) : EMPTY_PRS, + ); + const storedKey = useAppStore((state) => + taskId ? state.reviewPRSelection.selectedKeyByTaskId[taskId] : undefined, + ); + const setReviewPRSelection = useAppStore((state) => state.setReviewPRSelection); + const selectedPR = resolveReviewPRSelection(prs, storedKey); + const selectedKey = selectedPR ? prTaskKey(selectedPR) : null; + const selectPR = useCallback( + (pr: TaskPR) => { + if (taskId) setReviewPRSelection(taskId, prTaskKey(pr)); + }, + [taskId, setReviewPRSelection], + ); + + return { prs, selectedPR, selectedKey, selectPR }; +} diff --git a/apps/web/hooks/domains/session/use-review-sources.ts b/apps/web/hooks/domains/session/use-review-sources.ts index 9996665b02..0ad5982aa2 100644 --- a/apps/web/hooks/domains/session/use-review-sources.ts +++ b/apps/web/hooks/domains/session/use-review-sources.ts @@ -2,22 +2,25 @@ import { useEffect, useMemo } from "react"; import { useAppStore } from "@/components/state-provider"; import { useSessionGitStatus, useSessionGitStatusByRepo } from "./use-session-git-status"; import { useCumulativeDiff } from "./use-cumulative-diff"; -import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr"; +import { + findReviewPRByKey, + useReviewPRSelection, +} from "@/hooks/domains/github/use-review-pr-selection"; import { usePRDiff } from "@/hooks/domains/github/use-pr-diff"; +import { usePRReviewRepositoryIdentity } from "@/hooks/domains/github/use-pr-review-repository-identity"; import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories"; -import { useRepository } from "@/hooks/domains/workspace/use-repository"; import { getCumulativeReviewRepositoryNames, isReviewMultiRepo, normalizeDiffContent, - resolvePRReviewRepositoryName, reviewFileKey, splitReviewFileKey, } from "@/components/review/types"; import { createDebugLogger } from "@/lib/debug/log"; import type { ReviewFile } from "@/components/review/types"; -import type { PRDiffFile } from "@/lib/types/github"; +import type { PRDiffFile, TaskPR } from "@/lib/types/github"; import { normalizeFileChangeStatus } from "@/lib/utils/file-change-status"; +import { prTaskKey } from "@/components/github/pr-utils"; const debug = createDebugLogger("review:sources"); @@ -293,6 +296,12 @@ export type UseReviewSourcesResult = { hasPR: boolean; cumulativeLoading: boolean; prDiffLoading: boolean; + prDiffError: string | null; + refreshPRDiff: () => void; + prs: TaskPR[]; + selectedPR: TaskPR | null; + selectedPRKey: string | null; + selectPR: (pr: TaskPR) => void; /** * Files the backend dropped from the cumulative diff because the range * exceeded its file cap (large rebase). Zero when nothing was hidden; @@ -305,21 +314,21 @@ export type UseReviewSourcesResult = { * Raw PR diff files as ReviewFile[], NOT deduplicated with uncommitted/committed. * Use when displaying the PR-specific diff for a file (e.g. clicking a PR file row), * where the same file may also have local changes that would win deduplication in allFiles. - * NOTE: Only covers the primary PR (single-repo tasks). Multi-PR support is Bug 2. + * The selected PR is used unless file mode supplies an explicit PR key. */ rawPRFiles: ReviewFile[]; }; function useReviewRepositoryContext( - pr: ReturnType, + sessionId: string | null | undefined, + pr: TaskPR | null, gitStatus: ReturnType, statusByRepo: ReturnType, cumulativeDiff: ReturnType["diff"], ) { const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); const taskRepositories = useTaskRepositories(activeTaskId); - const prRepository = useRepository(pr?.repository_id ?? null); - const resolvedPRRepoName = resolvePRReviewRepositoryName(pr, prRepository?.name); + const resolvedPRRepoName = usePRReviewRepositoryIdentity(activeTaskId, sessionId, pr); return useMemo( () => normalizeReviewStatusSources({ @@ -341,18 +350,24 @@ function useReviewRepositoryContext( * Returns counts per source so consumers can render tab badges without * re-running the merge. */ -export function useReviewSources(sessionId: string | null | undefined): UseReviewSourcesResult { +export function useReviewSources( + sessionId: string | null | undefined, + explicitPRKey?: string, +): UseReviewSourcesResult { const gitStatus = useSessionGitStatus(sessionId ?? null); const statusByRepo = useSessionGitStatusByRepo(sessionId ?? null); const { diff: cumulativeDiff, loading: cumulativeLoading } = useCumulativeDiff(sessionId ?? null); - const pr = useActiveTaskPR(); + const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); + const { prs, selectedPR, selectedKey, selectPR } = useReviewPRSelection(activeTaskId); + const pr = explicitPRKey ? findReviewPRByKey(prs, explicitPRKey) : selectedPR; const { normalizedGitStatus, normalizedStatusByRepo, prRepoName, useRepositoryKeys } = - useReviewRepositoryContext(pr, gitStatus, statusByRepo, cumulativeDiff); - const { files: prDiffFiles, loading: prDiffLoading } = usePRDiff( - pr?.owner ?? null, - pr?.repo ?? null, - pr?.pr_number ?? null, - ); + useReviewRepositoryContext(sessionId, pr, gitStatus, statusByRepo, cumulativeDiff); + const { + files: prDiffFiles, + loading: prDiffLoading, + error: prDiffError, + refresh: refreshPRDiff, + } = usePRDiff(pr?.owner ?? null, pr?.repo ?? null, pr?.pr_number ?? null, pr?.last_synced_at); const { allFiles, sourceCounts } = useMemo( () => @@ -421,6 +436,12 @@ export function useReviewSources(sessionId: string | null | undefined): UseRevie hasPR, cumulativeLoading, prDiffLoading, + prDiffError, + refreshPRDiff, + prs, + selectedPR: pr, + selectedPRKey: pr ? prTaskKey(pr) : selectedKey, + selectPR, gitStatus, rawPRFiles, truncatedFilesCount: readTruncatedFilesCount(cumulativeDiff), diff --git a/apps/web/hooks/use-review-sidebar-resize.test.ts b/apps/web/hooks/use-review-sidebar-resize.test.ts index bada8c8ae1..7f74ac4a61 100644 --- a/apps/web/hooks/use-review-sidebar-resize.test.ts +++ b/apps/web/hooks/use-review-sidebar-resize.test.ts @@ -182,3 +182,56 @@ describe("useReviewSidebarResize", () => { expect(result.current.width).toBe(600 - REVIEW_SIDEBAR_LIMITS.minDiffPaneWidth); }); }); + +describe("useReviewSidebarResize observer lifecycle", () => { + it("reattaches the resize observer when the review source replaces the container", () => { + const originalResizeObserver = globalThis.ResizeObserver; + const observed: Element[] = []; + let disconnectCount = 0; + + class MockResizeObserver implements ResizeObserver { + observe(target: Element) { + observed.push(target); + } + + unobserve() {} + + disconnect() { + disconnectCount += 1; + } + } + + Object.defineProperty(globalThis, "ResizeObserver", { + configurable: true, + writable: true, + value: MockResizeObserver, + }); + + try { + const firstElement = document.createElement("div"); + const secondElement = document.createElement("div"); + const ref = { current: firstElement } as React.MutableRefObject; + const { rerender, unmount } = renderHook( + ({ sourceKey }) => useReviewSidebarResize(ref, true, sourceKey), + { initialProps: { sourceKey: "session-a:pr-1" } }, + ); + + expect(observed).toEqual([firstElement]); + + ref.current = secondElement; + rerender({ sourceKey: "session-b:pr-1" }); + + expect(disconnectCount).toBe(1); + expect(observed).toEqual([firstElement, secondElement]); + + unmount(); + expect(disconnectCount).toBe(2); + } finally { + Object.defineProperty(globalThis, "ResizeObserver", { + configurable: true, + writable: true, + value: originalResizeObserver, + }); + } + }); +}); diff --git a/apps/web/hooks/use-review-sidebar-resize.ts b/apps/web/hooks/use-review-sidebar-resize.ts index 607497ee48..1002284544 100644 --- a/apps/web/hooks/use-review-sidebar-resize.ts +++ b/apps/web/hooks/use-review-sidebar-resize.ts @@ -53,6 +53,7 @@ function restoreBodyStyles() { export function useReviewSidebarResize( containerRef?: React.RefObject, open = true, + sourceKey?: string, ) { const [width, setWidth] = useState(readStoredWidth); const isDragging = useRef(false); @@ -65,7 +66,8 @@ export function useReviewSidebarResize( // because the container element only exists once Radix mounts the // DialogContent portal — on the closed → open transition, `containerRef` // (a stable ref *object*) doesn't change identity, so the effect needs - // `open` to re-fire and pick up the now-populated `current`. + // `open` to re-fire and pick up the now-populated `current`. `sourceKey` + // likewise reattaches the observer when React replaces a keyed container. useEffect(() => { if (!open) return; const el = containerRef?.current; @@ -79,7 +81,7 @@ export function useReviewSidebarResize( const ro = new ResizeObserver(reclamp); ro.observe(el); return () => ro.disconnect(); - }, [containerRef, open]); + }, [containerRef, open, sourceKey]); const handleMouseDown = useCallback( (e: React.MouseEvent) => { diff --git a/apps/web/lib/state/default-state.ts b/apps/web/lib/state/default-state.ts index bae83ad461..4f34968370 100644 --- a/apps/web/lib/state/default-state.ts +++ b/apps/web/lib/state/default-state.ts @@ -99,6 +99,7 @@ export const defaultState = { mobileKanban: defaultUIState.mobileKanban, mobileSession: defaultUIState.mobileSession, chatInput: defaultUIState.chatInput, + reviewPRSelection: defaultUIState.reviewPRSelection, documentPanel: defaultUIState.documentPanel, systemHealth: defaultUIState.systemHealth, quickChat: defaultUIState.quickChat, @@ -198,13 +199,28 @@ function mergeSidebarTaskPrefsState( }; } +function mergeReviewPRSelectionState( + initialState: Partial, +): DefaultState["reviewPRSelection"] { + return { + ...defaultState.reviewPRSelection, + ...initialState.reviewPRSelection, + selectedKeyByTaskId: { + ...defaultState.reviewPRSelection.selectedKeyByTaskId, + ...initialState.reviewPRSelection?.selectedKeyByTaskId, + }, + }; +} + +function mergeSessionFailureNotification(initialState: Partial) { + return initialState.sessionFailureNotification ?? defaultState.sessionFailureNotification; +} + export function mergeInitialState(initialState?: Partial): DefaultState { if (!initialState) return defaultState; - return { ...defaultState, ...initialState, - // Ensure nested objects are properly merged kanban: { ...defaultState.kanban, ...initialState.kanban }, kanbanMulti: { ...defaultState.kanbanMulti, ...initialState.kanbanMulti }, workflows: { ...defaultState.workflows, ...initialState.workflows }, @@ -288,11 +304,11 @@ export function mergeInitialState(initialState?: Partial): Default mobileKanban: { ...defaultState.mobileKanban, ...initialState.mobileKanban }, mobileSession: { ...defaultState.mobileSession, ...initialState.mobileSession }, chatInput: { ...defaultState.chatInput, ...initialState.chatInput }, + reviewPRSelection: mergeReviewPRSelectionState(initialState), documentPanel: { ...defaultState.documentPanel, ...initialState.documentPanel }, systemHealth: { ...defaultState.systemHealth, ...initialState.systemHealth }, quickChat: mergeQuickChatState(initialState), - sessionFailureNotification: - initialState.sessionFailureNotification ?? defaultState.sessionFailureNotification, + sessionFailureNotification: mergeSessionFailureNotification(initialState), bottomTerminal: { ...defaultState.bottomTerminal, ...initialState.bottomTerminal }, sidebarViews: mergeSidebarViewState(initialState), sidebarTaskPrefs: mergeSidebarTaskPrefsState(initialState), diff --git a/apps/web/lib/state/dockview-panel-actions.test.ts b/apps/web/lib/state/dockview-panel-actions.test.ts index 3c33d13535..be1b10f2be 100644 --- a/apps/web/lib/state/dockview-panel-actions.test.ts +++ b/apps/web/lib/state/dockview-panel-actions.test.ts @@ -341,6 +341,23 @@ describe("addFileDiffPanel — preview behavior", () => { expect(preview.isActive).toBe(true); }); + it("does not reuse a pinned diff for the same path from a different PR", () => { + actions.addFileDiffPanel(SHARED_PATH, { + pin: true, + repositoryName: "backend", + prKey: "acme/backend/41", + }); + + actions.addFileDiffPanel(SHARED_PATH, { + repositoryName: "backend", + prKey: "acme/backend/42", + }); + + const preview = api.getPanel(PREVIEW_DIFF_ID) as unknown as MockPanel; + expect(preview).toBeDefined(); + expect(preview.params.prKey).toBe("acme/backend/42"); + }); + it("promotePreviewToPinned sets promoted flag on the preview diff", () => { actions.addFileDiffPanel(PATH_A); actions.promotePreviewToPinned("file-diff"); diff --git a/apps/web/lib/state/dockview-panel-actions.ts b/apps/web/lib/state/dockview-panel-actions.ts index 923040150f..c7d087cbe4 100644 --- a/apps/web/lib/state/dockview-panel-actions.ts +++ b/apps/web/lib/state/dockview-panel-actions.ts @@ -318,11 +318,12 @@ function buildFileDiffAction(get: StoreGet) { groupId?: string; source?: string; repositoryName?: string; + prKey?: string; }, ) => { const { api, centerGroupId } = get(); if (!api) return; - const itemId = buildRepoScopedItemId(path, opts?.repositoryName); + const itemId = buildRepoScopedItemId(path, opts?.prKey ?? opts?.repositoryName); openOrReplacePreview({ api, type: "file-diff", @@ -334,6 +335,7 @@ function buildFileDiffAction(get: StoreGet) { content: opts?.content, source: opts?.source, repositoryName: opts?.repositoryName, + prKey: opts?.prKey, }, groupId: opts?.groupId ?? centerGroupId, quiet: opts?.quiet, diff --git a/apps/web/lib/state/dockview-store.ts b/apps/web/lib/state/dockview-store.ts index 59c9a61534..c5e9854c93 100644 --- a/apps/web/lib/state/dockview-store.ts +++ b/apps/web/lib/state/dockview-store.ts @@ -144,6 +144,7 @@ type DockviewStore = { groupId?: string; source?: string; repositoryName?: string; + prKey?: string; }, ) => void; addCommitDetailPanel: ( diff --git a/apps/web/lib/state/slices/ui/types.ts b/apps/web/lib/state/slices/ui/types.ts index d3797d0fd1..5aac777813 100644 --- a/apps/web/lib/state/slices/ui/types.ts +++ b/apps/web/lib/state/slices/ui/types.ts @@ -53,6 +53,10 @@ export type ChatInputState = { planModeBySessionId: Record; }; +export type ReviewPRSelectionState = { + selectedKeyByTaskId: Record; +}; + export type ActiveDocument = | { type: "plan"; taskId: string } | { type: "file"; path: string; name: string }; @@ -143,6 +147,7 @@ export type UISliceState = { mobileKanban: MobileKanbanState; mobileSession: MobileSessionState; chatInput: ChatInputState; + reviewPRSelection: ReviewPRSelectionState; documentPanel: DocumentPanelState; systemHealth: SystemHealthState; quickChat: QuickChatState; @@ -190,6 +195,7 @@ export type UISliceActions = { setMobileSessionReview: (sessionId: string, mrKey: string | null) => void; setMobileSessionTaskSwitcherOpen: (open: boolean) => void; setPlanMode: (sessionId: string, enabled: boolean) => void; + setReviewPRSelection: (taskId: string, selectedKey: string) => void; setActiveDocument: (sessionId: string, doc: ActiveDocument | null) => void; setSystemHealth: (response: SystemHealthResponse) => void; setSystemHealthLoading: (loading: boolean) => void; diff --git a/apps/web/lib/state/slices/ui/ui-slice.ts b/apps/web/lib/state/slices/ui/ui-slice.ts index e1605a2851..d236f2b3c8 100644 --- a/apps/web/lib/state/slices/ui/ui-slice.ts +++ b/apps/web/lib/state/slices/ui/ui-slice.ts @@ -78,6 +78,7 @@ export const defaultUIState: UISliceState = { isTaskSwitcherOpen: false, }, chatInput: { planModeBySessionId: {} }, + reviewPRSelection: { selectedKeyByTaskId: {} }, documentPanel: { activeDocumentBySessionId: {} }, systemHealth: { issues: [], checks: [], healthy: true, loaded: false, loading: false }, quickChat: { isOpen: false, sessions: [], activeSessionId: null }, @@ -409,6 +410,10 @@ export const createUISlice: StateCreator { draft.chatInput.planModeBySessionId[sessionId] = enabled; }), + setReviewPRSelection: (taskId, selectedKey) => + set((draft) => { + draft.reviewPRSelection.selectedKeyByTaskId[taskId] = selectedKey; + }), setActiveDocument: (sessionId, doc) => set((draft) => { draft.documentPanel.activeDocumentBySessionId[sessionId] = doc; diff --git a/apps/web/lib/state/store-overrides.ts b/apps/web/lib/state/store-overrides.ts index 3f0c3c046c..1d948c62f4 100644 --- a/apps/web/lib/state/store-overrides.ts +++ b/apps/web/lib/state/store-overrides.ts @@ -80,6 +80,7 @@ export function buildStateOverrides(m: DefaultState) { mobileKanban: m.mobileKanban, mobileSession: m.mobileSession, chatInput: m.chatInput, + reviewPRSelection: m.reviewPRSelection, documentPanel: m.documentPanel, systemHealth: m.systemHealth, quickChat: m.quickChat, diff --git a/apps/web/lib/state/store.test.ts b/apps/web/lib/state/store.test.ts index cae6284299..787bc500a7 100644 --- a/apps/web/lib/state/store.test.ts +++ b/apps/web/lib/state/store.test.ts @@ -28,4 +28,35 @@ describe("createAppStore", () => { expect(store.getState().sidebarViews.activeViewId).toBe("server"); expect(store.getState().sidebarTaskPrefs.pinnedTaskIds).toEqual(["task-1"]); }); + + it("starts with no task-scoped Review PR overrides", () => { + const store = createAppStore(); + + expect(store.getState().reviewPRSelection).toEqual({ selectedKeyByTaskId: {} }); + }); + + it("retains caller-supplied task-scoped Review PR overrides", () => { + const store = createAppStore({ + reviewPRSelection: { + selectedKeyByTaskId: { "task-1": "acme/widget/7" }, + }, + } as Partial); + + expect(store.getState().reviewPRSelection.selectedKeyByTaskId).toEqual({ + "task-1": "acme/widget/7", + }); + }); + + it("stores Review PR overrides independently by task", () => { + const store = createAppStore(); + const state = store.getState(); + + state.setReviewPRSelection("task-1", "acme/widget/1"); + state.setReviewPRSelection("task-2", "acme/widget/2"); + + expect(store.getState().reviewPRSelection.selectedKeyByTaskId).toEqual({ + "task-1": "acme/widget/1", + "task-2": "acme/widget/2", + }); + }); }); diff --git a/apps/web/lib/state/store.ts b/apps/web/lib/state/store.ts index 74e8a9fdeb..d04099cfc8 100644 --- a/apps/web/lib/state/store.ts +++ b/apps/web/lib/state/store.ts @@ -237,6 +237,7 @@ export type AppState = { mobileKanban: (typeof defaultUIState)["mobileKanban"]; mobileSession: (typeof defaultUIState)["mobileSession"]; chatInput: (typeof defaultUIState)["chatInput"]; + reviewPRSelection: (typeof defaultUIState)["reviewPRSelection"]; documentPanel: (typeof defaultUIState)["documentPanel"]; systemHealth: (typeof defaultUIState)["systemHealth"]; quickChat: (typeof defaultUIState)["quickChat"]; @@ -381,6 +382,7 @@ export type AppState = { setMobileSessionReview: (sessionId: string, mrKey: string | null) => void; setMobileSessionTaskSwitcherOpen: (open: boolean) => void; setPlanMode: (sessionId: string, enabled: boolean) => void; + setReviewPRSelection: UIA["setReviewPRSelection"]; setActiveDocument: (sessionId: string, doc: UISliceTypes.ActiveDocument | null) => void; setSystemHealth: (response: SystemHealthResponse) => void; setSystemHealthLoading: (loading: boolean) => void; diff --git a/docs/plans/multi-pr-review-selector/plan.md b/docs/plans/multi-pr-review-selector/plan.md new file mode 100644 index 0000000000..b65f8c953f --- /dev/null +++ b/docs/plans/multi-pr-review-selector/plan.md @@ -0,0 +1,43 @@ +# Multi-PR Review selector + +**Spec:** [Multi-branch tasks](../../specs/tasks/multi-branch/spec.md) + +## Outcome + +Replace Review's implicit `prs[0]` behavior with a task-scoped selected PR. Keep one coherent PR revision visible at a time, preserve local/committed source precedence, and make selection reachable on desktop, phone, and tablet. + +## Decisions + +- Default to `getPrimaryTaskPR()` for compatibility. +- Store only an in-session override keyed by task ID and stable owner/repo/number key. +- Do not aggregate sibling PR diffs; review and comment identity is not PR-qualified. +- Exact PR timeline rows carry PR identity through desktop and mobile diff routing. +- No production backend change. + +## Waves + +### Wave 1 + +- [Task 01 — selection foundation](task-01-selection-foundation.md) — done + +### Wave 2 + +- [Task 02 — Review selector UI](task-02-review-selector-ui.md) — done; depends on Task 01 +- [Task 03 — inline review and PR routing](task-03-inline-review-routing.md) — done; depends on Task 01 + +Tasks 02 and 03 own disjoint production files and may run in parallel after Task 01. + +### Wave 3 + +- [Task 04 — E2E, QA, and verification](task-04-e2e-verification.md) — done; depends on Tasks 02 and 03 + +## Verification + +From `apps/`, run focused Vitest files listed in each task. From `apps/web/`, run the two focused E2E specs. Finish from repository root with `make fmt`, then `make typecheck test lint`. + +## Risks + +- Switching clears PR files while fetching; Review must not auto-close during that transient empty state. +- Deferred requests must not flash files from the prior PR. +- Same-path review marks/comments remain current-visible-revision semantics, not independent per-PR history. +- Review toolbars need intentional phone wrapping, 44px touch targets, one content scroller, and no horizontal page overflow. diff --git a/docs/plans/multi-pr-review-selector/task-01-selection-foundation.md b/docs/plans/multi-pr-review-selector/task-01-selection-foundation.md new file mode 100644 index 0000000000..8d7601060a --- /dev/null +++ b/docs/plans/multi-pr-review-selector/task-01-selection-foundation.md @@ -0,0 +1,46 @@ +--- +id: multi-pr-review-01 +title: Add task-scoped Review PR selection +status: done +wave: 1 +depends_on: [] +plan: ./plan.md +--- + +## Acceptance + +- Resolver defaults to primary PR, honors a stable override, isolates tasks, and falls back when selected PR disappears. +- Zustand UI state stores only task-keyed in-session overrides. +- PR key utility lives outside React panel code. + +## Files + +- `apps/web/components/github/pr-utils.ts` +- Existing `prTaskKey` importers +- `apps/web/hooks/domains/github/use-review-pr-selection.ts` +- `apps/web/hooks/domains/github/use-review-pr-selection.test.ts` +- `apps/web/hooks/domains/github/use-pr-review-repository-identity.ts` +- `apps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsx` +- `apps/web/lib/state/slices/ui/types.ts` +- `apps/web/lib/state/slices/ui/ui-slice.ts` +- `apps/web/lib/state/default-state.ts` +- `apps/web/lib/state/store.ts` +- `apps/web/lib/state/store-overrides.ts` + +## Inputs + +- Spec Frontend section +- `useTaskPR()` / `getPrimaryTaskPR()` in `use-task-pr.ts` +- Derived fallback pattern in `multi-pr-ci-popover.tsx` + +## Method + +Use strict RED → GREEN → REFACTOR. First run must fail on behavior, not missing import. + +## Verification + +`cd apps/web && pnpm test -- hooks/domains/github/use-review-pr-selection.test.ts hooks/domains/github/use-pr-review-repository-identity.test.tsx lib/state/store.test.ts` + +## Output contract + +Report summary, files, RED and GREEN results, blockers, and remaining risks; mark status done only after focused verification passes. diff --git a/docs/plans/multi-pr-review-selector/task-02-review-selector-ui.md b/docs/plans/multi-pr-review-selector/task-02-review-selector-ui.md new file mode 100644 index 0000000000..237da4e716 --- /dev/null +++ b/docs/plans/multi-pr-review-selector/task-02-review-selector-ui.md @@ -0,0 +1,42 @@ +--- +id: multi-pr-review-02 +title: Add selected-PR controls to expanded Review +status: done +wave: 2 +depends_on: [multi-pr-review-01] +plan: ./plan.md +--- + +## Acceptance + +- Review shows selector only for two or more PRs and switches files atomically with loading, empty, error, and retry states. +- Switch resets stale file/filter/scroll state and never closes Review during fetch. +- Desktop, phone, and tablet share behavior; phone menu has 44px targets and bounded internal scrolling. + +## Files + +- `apps/web/components/task/use-review-dialog.ts` +- `apps/web/components/task/dockview-review-dialog.tsx` +- Desktop/mobile/tablet task layout mounts +- `apps/web/components/review/review-dialog.tsx` +- `apps/web/components/review/review-top-bar.tsx` +- New `apps/web/components/review/review-pr-selector.tsx` +- Focused Review unit tests + +## Inputs + +- Task 01 hook/state +- `pr-topbar-button.tsx` menu pattern +- Mobile global menu containment and mobile-parity contract + +## Method + +Use RED → GREEN → REFACTOR. Test pure state/file behavior first; use Playwright in Task 04 for rendered interaction. + +## Verification + +`cd apps && pnpm --filter @kandev/web test -- --run components/task/use-review-dialog.test.ts components/review/review-dialog.build-files.test.ts` + +## Output contract + +Report summary, files, RED and GREEN results, responsive choices, blockers, and risks; mark status done only after focused verification passes. diff --git a/docs/plans/multi-pr-review-selector/task-03-inline-review-routing.md b/docs/plans/multi-pr-review-selector/task-03-inline-review-routing.md new file mode 100644 index 0000000000..4f166f0ff1 --- /dev/null +++ b/docs/plans/multi-pr-review-selector/task-03-inline-review-routing.md @@ -0,0 +1,42 @@ +--- +id: multi-pr-review-03 +title: Route inline review through exact selected PR +status: done +wave: 2 +depends_on: [multi-pr-review-01] +plan: ./plan.md +--- + +## Acceptance + +- Inline review sources and progress use selected PR instead of task primary. +- Every PR timeline file carries stable PR identity through dockview and mobile sheet routes. +- Same-repository, same-path sibling rows render the clicked PR patch. + +## Files + +- `apps/web/hooks/domains/session/use-review-sources.ts` +- `apps/web/components/task/task-changes-panel.tsx` +- `apps/web/components/task/changes-top-bar.tsx` +- Changes data/helper/timeline/PR-file modules and tests +- `apps/web/components/task/changes-diff-target.ts` +- Dockview panel/store routing modules and tests +- Mobile changes panel/diff sheet + +## Inputs + +- Task 01 hook/state +- Existing `useActiveTaskPRsWithFiles()` fan-out +- PR-only raw file behavior in `TaskChangesPanel` + +## Method + +Use RED → GREEN → REFACTOR for selection, progress, and identity propagation. + +## Verification + +`cd apps && pnpm --filter @kandev/web test -- --run hooks/domains/session/use-review-sources.test.ts components/task/changes-panel-helpers.test.ts components/task/changes-panel-pr-files.test.tsx components/task/dockview-panel-content.diff.test.tsx lib/state/dockview-panel-actions.test.ts` + +## Output contract + +Report summary, files, RED and GREEN results, blockers, and risks; mark status done only after focused verification passes. diff --git a/docs/plans/multi-pr-review-selector/task-04-e2e-verification.md b/docs/plans/multi-pr-review-selector/task-04-e2e-verification.md new file mode 100644 index 0000000000..39a69f7baf --- /dev/null +++ b/docs/plans/multi-pr-review-selector/task-04-e2e-verification.md @@ -0,0 +1,37 @@ +--- +id: multi-pr-review-04 +title: Prove multi-PR Review across viewports +status: done +wave: 3 +depends_on: [multi-pr-review-02, multi-pr-review-03] +plan: ./plan.md +--- + +## Acceptance + +- Desktop E2E switches two same-repository PRs and proves no stale or wrong-path diff. +- Mobile E2E proves bottom-menu selection, 44px targets, viewport containment, and no document overflow. +- Tablet smoke opens Review; full format, typecheck, test, and lint pass. + +## Files + +- `apps/web/e2e/tests/review/review-multi-pr.spec.ts` +- `apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts` +- E2E page/helper files only when reuse warrants it +- Mock controller only if fixture realism requires it + +## Inputs + +- Tasks 02 and 03 +- Existing review-file-status and pr-multi-popover specs + +## Verification + +- `cd apps/web && pnpm e2e:run tests/review/review-multi-pr.spec.ts` +- `cd apps/web && pnpm e2e:run --project mobile-chrome tests/review/mobile-review-multi-pr.spec.ts` +- `make fmt` +- `make typecheck test lint` + +## Output contract + +Report E2E/visual evidence, full verification, remaining risks, and all files; mark status done only after all required checks pass or record exact external blocker. diff --git a/docs/public/sessions-and-review.md b/docs/public/sessions-and-review.md index 62a6fbba5d..69ff679ed6 100644 --- a/docs/public/sessions-and-review.md +++ b/docs/public/sessions-and-review.md @@ -111,6 +111,8 @@ From this panel you can stage or unstage files, discard working-tree changes, co Select **Review** in the Changes header. Kandev builds a repository-aware file list by merging available uncommitted, cumulative committed, and linked-PR files. When a path occurs in more than one source, the uncommitted version wins deduplication. +When a task has multiple linked pull requests, use the PR selector in the Changes diff header or Review toolbar to inspect one PR revision at a time. The selection is scoped to that task for the current app session. Switching PRs replaces only the remote PR contribution; uncommitted and committed sources keep their normal precedence. Selecting a file from a specific PR row opens that exact PR revision, even when a sibling PR changes the same path. + branch tabs are deferred — agents drive multi-branch via the MCP tool today. ## Non-goals @@ -56,6 +59,8 @@ Workarounds (sibling tasks, manually managing two worktrees) lost shared context - **Auto-stack PRs.** Multi-branch lets you open N PRs; it does not detect base/branch relationships and stack them. Users do that themselves. - **Cross-branch merge orchestration.** Each branch's PR lifecycle is independent. - **Branch deletion / cleanup automation.** A `RemoveBranchFromTask` symmetric service method is planned but not in v1. +- **Aggregate multi-PR review.** Review does not merge sibling PR diffs into one file list because two PRs can carry different revisions of the same repository path. +- **Independent per-PR review history.** Reviewed-file and pending-comment identity remain session/repository/path scoped. Switching PRs treats a different diff hash as a new visible revision; PR-qualified persistence is separate data-model work. ## Risks @@ -72,6 +77,8 @@ Workarounds (sibling tasks, manually managing two worktrees) lost shared context - `TestAddBranchToTask_HappyPath` — second branch appended after the fact lands as a new row. - `TestAddBranchToTask_RejectsDuplicate` — re-adding the same `(repo, branch)` errors. - `TestLaunchPreparedSession_MultiBranch_ReusesWorktreeIDsByBranchSlug` — a follow-on session for the same task reuses each existing branch worktree instead of preparing a new task directory. +- Web unit tests prove selected-PR default, override, task isolation, and removed-PR fallback behavior. +- Desktop and mobile Playwright tests prove a two-PR task can switch Review from the primary PR to a sibling PR without stale files, overflow, or closing the surface. ## Open questions