diff --git a/apps/web/components/diff/diff-viewer-resolver.tsx b/apps/web/components/diff/diff-viewer-resolver.tsx index afa32cda0b..7cad22e4b8 100644 --- a/apps/web/components/diff/diff-viewer-resolver.tsx +++ b/apps/web/components/diff/diff-viewer-resolver.tsx @@ -27,6 +27,8 @@ interface DiffViewerResolverProps { onPreviewMarkdown?: (filePath: string) => void; onRevert?: (filePath: string) => void; enableAcceptReject?: boolean; + /** Render and navigate to the active task walkthrough (Pierre diffs only). */ + enableWalkthroughAnnotations?: boolean; onRevertBlock?: (filePath: string, info: RevertBlockInfo) => void; wordWrap?: boolean; editable?: boolean; @@ -58,6 +60,7 @@ export const DiffViewerResolved = memo(function DiffViewerResolved(props: DiffVi enableComments, baseRef, enableExpansion, + enableWalkthroughAnnotations, expandUnchanged, onToggleExpandUnchanged, onPreviewMarkdown, diff --git a/apps/web/components/diff/diff-viewer.test.tsx b/apps/web/components/diff/diff-viewer.test.tsx index 3fb9104d96..3acb6c8b3e 100644 --- a/apps/web/components/diff/diff-viewer.test.tsx +++ b/apps/web/components/diff/diff-viewer.test.tsx @@ -84,6 +84,17 @@ describe("DiffViewer", () => { await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(renderCount)); }); + it("rerenders when walkthrough annotations are disabled", async () => { + const { rerender } = render(); + + await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(0)); + const renderCount = fileDiffProps.length; + + rerender(); + + await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(renderCount)); + }); + it("keeps simple hunk separators when full-content reparse rejects expansion", async () => { expansionState.canExpand = false; diff --git a/apps/web/components/diff/diff-viewer.tsx b/apps/web/components/diff/diff-viewer.tsx index 0f92856bfb..ce6258778b 100644 --- a/apps/web/components/diff/diff-viewer.tsx +++ b/apps/web/components/diff/diff-viewer.tsx @@ -36,6 +36,8 @@ interface DiffViewerProps { onPreviewMarkdown?: (filePath: string) => void; onRevert?: (filePath: string) => void; enableAcceptReject?: boolean; + /** Render and navigate to the active task walkthrough inside this diff. */ + enableWalkthroughAnnotations?: boolean; onRevertBlock?: (filePath: string, info: RevertBlockInfo) => Promise | void; wordWrap?: boolean; /** Enable diff expansion (show expand up/down buttons at hunk separators) */ @@ -68,6 +70,7 @@ const SCALAR_PROP_KEYS: (keyof DiffViewerProps)[] = [ "onPreviewMarkdown", "onRevert", "enableAcceptReject", + "enableWalkthroughAnnotations", "onRevertBlock", "wordWrap", "enableExpansion", @@ -271,6 +274,7 @@ export const DiffViewer = memo(function DiffViewer(props: DiffViewerProps) { onPreviewMarkdown, onRevert, enableAcceptReject = false, + enableWalkthroughAnnotations = true, onRevertBlock, wordWrap: wordWrapProp, enableExpansion = false, @@ -293,6 +297,7 @@ export const DiffViewer = memo(function DiffViewer(props: DiffViewerProps) { data, enableComments, enableAcceptReject, + enableWalkthroughAnnotations, sessionId, onCommentAdd, onCommentDelete, diff --git a/apps/web/components/diff/file-diff-viewer.tsx b/apps/web/components/diff/file-diff-viewer.tsx index 3197ad34f5..35002dd9bb 100644 --- a/apps/web/components/diff/file-diff-viewer.tsx +++ b/apps/web/components/diff/file-diff-viewer.tsx @@ -24,6 +24,8 @@ interface FileDiffViewerProps { onPreviewMarkdown?: (filePath: string) => void; onRevert?: (filePath: string) => void; enableAcceptReject?: boolean; + /** Render and navigate to the active task walkthrough inside this diff. */ + enableWalkthroughAnnotations?: boolean; onRevertBlock?: (filePath: string, info: RevertBlockInfo) => void; wordWrap?: boolean; /** Enable diff expansion (show expand up/down buttons at hunk separators) */ @@ -70,6 +72,7 @@ export const FileDiffViewer = memo(function FileDiffViewer({ onPreviewMarkdown, onRevert, enableAcceptReject, + enableWalkthroughAnnotations, onRevertBlock, wordWrap, enableExpansion, @@ -102,6 +105,7 @@ export const FileDiffViewer = memo(function FileDiffViewer({ onPreviewMarkdown={onPreviewMarkdown} onRevert={onRevert} enableAcceptReject={enableAcceptReject} + enableWalkthroughAnnotations={enableWalkthroughAnnotations} onRevertBlock={onRevertBlock} wordWrap={wordWrap} enableExpansion={enableExpansion} diff --git a/apps/web/components/diff/use-diff-viewer-state.test.ts b/apps/web/components/diff/use-diff-viewer-state.test.ts index ddbe3babb1..e0dbeaa806 100644 --- a/apps/web/components/diff/use-diff-viewer-state.test.ts +++ b/apps/web/components/diff/use-diff-viewer-state.test.ts @@ -115,11 +115,13 @@ describe("buildHunkAnnotations", () => { }); }); +const WALKTHROUGH_PATH = "apps/web/main.ts"; + describe("buildWalkthroughSelectedLines", () => { it("returns the active walkthrough line range for a matching file", () => { expect( buildWalkthroughSelectedLines( - { path: "apps/web/main.ts", repository_name: "frontend" }, + { path: WALKTHROUGH_PATH, repository_name: "frontend" }, { file: "main.ts", repo: "frontend", line: 10, line_end: 12, text: "explain" }, ), ).toEqual({ side: "additions", start: 10, end: 12 }); @@ -128,7 +130,7 @@ describe("buildWalkthroughSelectedLines", () => { it("returns null for mismatched repos", () => { expect( buildWalkthroughSelectedLines( - { path: "apps/web/main.ts", repository_name: "backend" }, + { path: WALKTHROUGH_PATH, repository_name: "backend" }, { file: "main.ts", repo: "frontend", line: 10, text: "explain" }, ), ).toBeNull(); @@ -137,9 +139,19 @@ describe("buildWalkthroughSelectedLines", () => { it("defaults line_end to line when the walkthrough step has no line_end", () => { expect( buildWalkthroughSelectedLines( - { path: "apps/web/main.ts", repository_name: "frontend" }, + { path: WALKTHROUGH_PATH, repository_name: "frontend" }, { file: "main.ts", repo: "frontend", line: 15, text: "see this" }, ), ).toEqual({ side: "additions", start: 15, end: 15 }); }); + + it("returns null when walkthrough annotations are disabled", () => { + expect( + buildWalkthroughSelectedLines( + { path: WALKTHROUGH_PATH, repository_name: "frontend" }, + { file: "main.ts", repo: "frontend", line: 15, text: "see this" }, + false, + ), + ).toBeNull(); + }); }); diff --git a/apps/web/components/diff/use-diff-viewer-state.ts b/apps/web/components/diff/use-diff-viewer-state.ts index 2e83c571c0..49702a75fe 100644 --- a/apps/web/components/diff/use-diff-viewer-state.ts +++ b/apps/web/components/diff/use-diff-viewer-state.ts @@ -141,6 +141,8 @@ type UseDiffViewerStateOpts = { data: FileDiffData; enableComments: boolean; enableAcceptReject: boolean; + /** Render and navigate to the active task walkthrough inside this diff. */ + enableWalkthroughAnnotations?: boolean; sessionId?: string; onCommentAdd?: (comment: DiffComment) => void; onCommentDelete?: (commentId: string) => void; @@ -164,8 +166,9 @@ type UseDiffViewerStateOpts = { export function buildWalkthroughSelectedLines( file: WalkthroughTargetFile, step: WalkthroughStep | null | undefined, + enabled = true, ): SelectedLineRange | null { - if (!step || !walkthroughStepMatchesFile(file, step)) return null; + if (!enabled || !step || !walkthroughStepMatchesFile(file, step)) return null; return { side: "additions", start: step.line, @@ -176,17 +179,18 @@ export function buildWalkthroughSelectedLines( function useWalkthroughSelection( filePath: string, repo: string | undefined, + enabled: boolean, ): { annotation: DiffLineAnnotation | null; selectedLines: SelectedLineRange | null; } { - const activeTaskId = useOptionalAppStore((s) => s.tasks.activeTaskId, null); + const activeTaskId = useOptionalAppStore((s) => (enabled ? s.tasks.activeTaskId : null), null); const walkthrough = useOptionalAppStore( - (s) => (activeTaskId ? s.walkthroughs.byTaskId[activeTaskId] : null), + (s) => (enabled && activeTaskId ? s.walkthroughs.byTaskId[activeTaskId] : null), null, ); const activeStep = useOptionalAppStore( - (s) => (activeTaskId ? (s.walkthroughs.activeStepByTaskId[activeTaskId] ?? 0) : 0), + (s) => (enabled && activeTaskId ? (s.walkthroughs.activeStepByTaskId[activeTaskId] ?? 0) : 0), 0, ); return useMemo(() => { @@ -194,6 +198,7 @@ function useWalkthroughSelection( const selectedLines = buildWalkthroughSelectedLines( { path: filePath, repository_name: repo }, step, + enabled, ); if (!selectedLines) return { annotation: null, selectedLines: null }; return { @@ -204,7 +209,7 @@ function useWalkthroughSelection( }, selectedLines, }; - }, [walkthrough, activeStep, filePath, repo]); + }, [walkthrough, activeStep, filePath, repo, enabled]); } function useDiffViewerAnnotations({ @@ -217,12 +222,14 @@ function useDiffViewerAnnotations({ filePath, repo, diff, + enableWalkthroughAnnotations, changeLineMapRef, revertInfoRef, }: BuildAnnotationsOpts & { filePath: string; repo?: string; diff?: string; + enableWalkthroughAnnotations: boolean; changeLineMapRef: RefObject>; revertInfoRef: RefObject>; }) { @@ -246,7 +253,7 @@ function useDiffViewerAnnotations({ ], ); - const walkthrough = useWalkthroughSelection(filePath, repo); + const walkthrough = useWalkthroughSelection(filePath, repo, enableWalkthroughAnnotations); const reviewFindings = useReviewFindingAnnotations({ filePath, repo, diff }); const withWalkthrough = useMemo(() => { const combined = [...annotations, ...reviewFindings.annotations]; @@ -415,6 +422,7 @@ export function useDiffViewerState(opts: UseDiffViewerStateOpts) { data, enableComments, enableAcceptReject, + enableWalkthroughAnnotations = true, sessionId, onCommentAdd, onCommentDelete, @@ -465,6 +473,7 @@ export function useDiffViewerState(opts: UseDiffViewerStateOpts) { filePath: data.filePath, repo, diff: data.diff, + enableWalkthroughAnnotations, changeLineMapRef, revertInfoRef, }); diff --git a/apps/web/components/review/review-dialog-surface.tsx b/apps/web/components/review/review-dialog-surface.tsx index c621705511..a64243373f 100644 --- a/apps/web/components/review/review-dialog-surface.tsx +++ b/apps/web/components/review/review-dialog-surface.tsx @@ -61,6 +61,7 @@ function ReviewDialogDiffContent({ sessionId={sessionId} autoMarkOnScroll={state.autoMarkOnScroll} wordWrap={state.wordWrap} + enableWalkthroughAnnotations={false} onToggleReviewed={state.handleToggleReviewed} onDiscard={state.handleDiscard} onOpenFile={onOpenFile} diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index b499992868..194863591b 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, useMemo, useCallback, createRef, useState, useEffect } from "react"; +import { memo, useMemo, useCallback, createRef, useState } from "react"; import type { DiffComment } from "@/lib/diff/types"; import type { FileInfo, CumulativeDiff } from "@/lib/state/slices/session-runtime/types"; import type { PRDiffFile, TaskPR } from "@/lib/types/github"; @@ -8,7 +8,6 @@ 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 { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; import { DEFAULT_DIFF_WORD_WRAP } from "@/components/diff/diff-defaults"; @@ -451,35 +450,6 @@ 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 - * its diff expands and the inline WalkthroughStepCard renders at the line. - */ -function useWalkthroughFileSelection( - open: boolean, - allFiles: ReviewFile[], - filter: string, - setFilter: (value: string) => void, - handleSelectFile: (key: string) => void, -) { - const step = useAppStore((state) => { - const taskId = state.tasks.activeTaskId; - if (!taskId) return null; - const wt = state.walkthroughs.byTaskId[taskId]; - const idx = state.walkthroughs.activeStepByTaskId[taskId] ?? 0; - return wt?.steps[idx] ?? null; - }); - useEffect(() => { - if (!open || !step) return; - const file = allFiles.find((f) => walkthroughStepMatchesFile(f, step, allFiles)); - if (!file) return; - const q = filter.trim().toLowerCase(); - if (q && !file.path.toLowerCase().includes(q)) setFilter(""); - handleSelectFile(reviewFileKey(file)); - }, [open, step, allFiles, filter, setFilter, handleSelectFile]); -} - function useReviewDialogWalkthroughRequest({ sessionId, allFiles, @@ -514,7 +484,6 @@ export const ReviewDialog = memo(function ReviewDialog(props: ReviewDialogProps) sessionId, allFiles: s.allFiles, }); - useWalkthroughFileSelection(open, s.allFiles, s.filter, s.setFilter, s.handleSelectFile); return ( = []; + +vi.mock("@/components/diff", () => ({ + FileDiffViewer: (props: { enableWalkthroughAnnotations?: boolean; filePath: string }) => { + diffViewerProps.push(props); + return
{props.filePath}
; + }, + DiffErrorBoundary: ({ children }: { children: ReactNode }) => <>{children}, +})); +vi.mock("@/components/editors/file-actions-dropdown", () => ({ + FileActionsDropdown: () => null, + FileActionsMenuItems: () => null, +})); +vi.mock("@/components/editors/external-vcs-file-link", () => ({ + ExternalVcsFileLink: () => null, + ExternalVcsFileMenuItem: () => null, +})); +vi.mock("@/hooks/use-responsive-breakpoint", () => ({ + useResponsiveBreakpoint: () => ({ isMobile: false }), +})); +vi.mock("@/lib/ws/connection", () => ({ getWebSocketClient: () => null })); +vi.mock("@/lib/ws/workspace-files", () => ({ + requestFileContent: vi.fn(), + updateFileContent: vi.fn(), +})); +vi.mock("@/hooks/use-global-view-mode", () => ({ + useGlobalViewMode: () => ["unified", vi.fn()], +})); +vi.mock("@/hooks/domains/comments/use-run-comment", () => ({ + useRunComment: () => ({ runComment: vi.fn() }), +})); +vi.mock("@/hooks/domains/session/use-base-branch-by-repo", () => ({ + useBaseBranchByRepo: () => ({}), +})); +vi.mock("@/hooks/domains/github/use-task-pr", () => ({ + useActiveTaskPR: () => null, +})); +vi.mock("@/components/state-provider", () => ({ useAppStore: () => null })); +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: vi.fn() }), +})); + +import { ReviewDiffList } from "./review-diff-list"; + +function file(path: string): ReviewFile { + return { + path, + diff: "@@ -1 +1 @@\n-a\n+b\n", + status: "modified", + additions: 1, + deletions: 1, + staged: false, + source: "uncommitted", + }; +} + +function installIntersectionObserver() { + class MockIntersectionObserver implements IntersectionObserver { + readonly root: Element | Document | null = null; + readonly rootMargin = "0px"; + readonly thresholds: readonly number[] = []; + private readonly record: ObserverRecord; + + constructor(callback: IntersectionObserverCallback, options?: IntersectionObserverInit) { + this.record = { callback, instance: this, options, targets: [] }; + observerRecords.push(this.record); + } + + disconnect() {} + observe(target: Element) { + this.record.targets.push(target); + } + takeRecords(): IntersectionObserverEntry[] { + return []; + } + unobserve() {} + } + + vi.stubGlobal("IntersectionObserver", MockIntersectionObserver); +} + +function firePassedTop(record: ObserverRecord, target: Element) { + const root = record.options?.root as HTMLElement; + Object.defineProperty(root, "getBoundingClientRect", { + configurable: true, + value: () => ({ top: 100 }), + }); + act(() => { + record.callback( + [ + { + isIntersecting: false, + boundingClientRect: { top: 50 }, + target, + } as IntersectionObserverEntry, + ], + record.instance, + ); + }); +} + +let scrollIntoViewDescriptor: PropertyDescriptor | undefined; + +beforeEach(() => { + observerRecords.length = 0; + diffViewerProps.length = 0; + installIntersectionObserver(); + scrollIntoViewDescriptor = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + "scrollIntoView", + ); + vi.stubGlobal("requestAnimationFrame", (callback: FrameRequestCallback) => { + callback(0); + return 1; + }); + Object.defineProperty(HTMLElement.prototype, "scrollIntoView", { + configurable: true, + value: vi.fn(), + }); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + if (scrollIntoViewDescriptor) { + Object.defineProperty(HTMLElement.prototype, "scrollIntoView", scrollIntoViewDescriptor); + } else { + Reflect.deleteProperty(HTMLElement.prototype, "scrollIntoView"); + } +}); + +describe("ReviewDiffList auto-mark", () => { + it("ignores a programmatic file jump, then resumes on manual scroll input", () => { + const files = [file("a.ts"), file("b.ts")]; + const onToggleReviewed = vi.fn(); + const refs = new Map(files.map((entry) => [entry.path, createRef()])); + const view = render( + + undefined} + fileRefs={refs} + /> + , + ); + const scrollRoot = view.container.firstElementChild as HTMLElement; + const firstAutoMarkObserver = observerRecords.find( + (record) => + record.options?.threshold === 0 && + record.targets[0]?.parentElement?.textContent?.includes("a.ts"), + ); + expect(firstAutoMarkObserver).toBeDefined(); + + firePassedTop(firstAutoMarkObserver!, firstAutoMarkObserver!.targets[0]); + expect(onToggleReviewed).not.toHaveBeenCalled(); + + fireEvent.wheel(scrollRoot); + firePassedTop(firstAutoMarkObserver!, firstAutoMarkObserver!.targets[0]); + expect(onToggleReviewed).toHaveBeenCalledWith("a.ts", true); + }); + + it("forwards disabled walkthrough annotations to file diffs", () => { + const entry = file("a.ts"); + render( + + undefined} + onDiscard={() => undefined} + fileRefs={new Map([[entry.path, createRef()]])} + /> + , + ); + + expect(diffViewerProps.at(-1)?.enableWalkthroughAnnotations).toBe(false); + }); +}); diff --git a/apps/web/components/review/review-diff-list-grouping.test.tsx b/apps/web/components/review/review-diff-list-grouping.test.tsx index 2c95d9f080..4a85240128 100644 --- a/apps/web/components/review/review-diff-list-grouping.test.tsx +++ b/apps/web/components/review/review-diff-list-grouping.test.tsx @@ -78,6 +78,7 @@ const baseProps = { sessionId: "sess", autoMarkOnScroll: false, wordWrap: false, + enableWalkthroughAnnotations: true, selectedFile: null, onToggleReviewed: () => undefined, onDiscard: () => undefined, diff --git a/apps/web/components/review/review-diff-list.tsx b/apps/web/components/review/review-diff-list.tsx index da3334ddd9..c0ee88bc14 100644 --- a/apps/web/components/review/review-diff-list.tsx +++ b/apps/web/components/review/review-diff-list.tsx @@ -25,6 +25,8 @@ import { ReviewMarkdownDiffPreviewContent } from "./review-markdown-diff-preview import { groupByRepositoryName } from "@/lib/group-by-repo"; import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr"; +const SCROLL_KEYS = new Set(["ArrowDown", "ArrowUp", "PageDown", "PageUp", "Home", "End", " "]); + type ReviewDiffListProps = { files: ReviewFile[]; reviewedFiles: Set; @@ -32,6 +34,7 @@ type ReviewDiffListProps = { sessionId: string; autoMarkOnScroll: boolean; wordWrap: boolean; + enableWalkthroughAnnotations: boolean; selectedFile?: string | null; onToggleReviewed: (path: string, reviewed: boolean) => void; onDiscard: (path: string) => void; @@ -47,6 +50,7 @@ export const ReviewDiffList = memo(function ReviewDiffList({ sessionId, autoMarkOnScroll, wordWrap, + enableWalkthroughAnnotations, selectedFile, onToggleReviewed, onDiscard, @@ -55,6 +59,16 @@ export const ReviewDiffList = memo(function ReviewDiffList({ fileRefs, }: ReviewDiffListProps) { const scrollContainerRef = useRef(null); + const suppressAutoMarkRef = useRef(false); + const allowAutoMark = useCallback(() => { + suppressAutoMarkRef.current = false; + }, []); + const handlePointerDown = useCallback((event: React.PointerEvent) => { + if (event.target === event.currentTarget) suppressAutoMarkRef.current = false; + }, []); + const handleKeyDown = useCallback((event: React.KeyboardEvent) => { + if (SCROLL_KEYS.has(event.key)) suppressAutoMarkRef.current = false; + }, []); // Resolve base branches once per list (not per row) — the value is identical // for every file. Only a single-repo task has an unambiguous fallback; with // multiple repos a committed file lacking `repository_name` must NOT borrow @@ -77,7 +91,15 @@ export const ReviewDiffList = memo(function ReviewDiffList({ const groups = useMemo(() => groupByRepositoryName(files, (f) => f.repository_name), [files]); const showRepoHeaders = groups.length > 1 || (groups[0]?.repositoryName ?? "") !== ""; return ( -
+
{groups.map((group) => (
= 0 && @@ -110,6 +133,7 @@ export const ReviewDiffList = memo(function ReviewDiffList({ onPreviewMarkdown={onPreviewMarkdown} sectionRef={fileRefs.get(key)} scrollContainer={scrollContainerRef} + suppressAutoMark={suppressAutoMarkRef} externalLinkContext={{ baseBranchByRepo, fallbackBaseBranch, @@ -140,6 +164,7 @@ type FileDiffSectionProps = { sessionId: string; autoMarkOnScroll: boolean; wordWrap: boolean; + enableWalkthroughAnnotations: boolean; isSelected?: boolean; forceLoad?: boolean; onToggleReviewed: (key: string, reviewed: boolean) => void; @@ -148,6 +173,7 @@ type FileDiffSectionProps = { onPreviewMarkdown?: (filePath: string, repo?: string) => void; sectionRef?: React.RefObject; scrollContainer: React.RefObject; + suppressAutoMark: React.RefObject; /** Per-repo base branches + single-repo fallback, resolved once by the list * and shared across rows so diff expansion can fetch the correct old side. */ externalLinkContext: ReviewExternalLinkContext; @@ -182,6 +208,7 @@ type AutoMarkArgs = { fileKey: string; onToggleReviewed: (key: string, reviewed: boolean) => void; scrollContainer: React.RefObject; + suppressAutoMark: React.RefObject; }; function useAutoMarkOnScroll({ @@ -191,6 +218,7 @@ function useAutoMarkOnScroll({ fileKey, onToggleReviewed, scrollContainer, + suppressAutoMark, }: AutoMarkArgs) { const scrollSentinelRef = useRef(null); const autoMarkedRef = useRef(false); @@ -207,6 +235,7 @@ function useAutoMarkOnScroll({ if ( !entry.isIntersecting && entry.boundingClientRect.top < root.getBoundingClientRect().top && + !suppressAutoMark.current && !autoMarkedRef.current ) { autoMarkedRef.current = true; @@ -218,7 +247,15 @@ function useAutoMarkOnScroll({ ); observer.observe(sentinel); return () => observer.disconnect(); - }, [autoMarkOnScroll, fileKey, isReviewed, isStale, onToggleReviewed, scrollContainer]); + }, [ + autoMarkOnScroll, + fileKey, + isReviewed, + isStale, + onToggleReviewed, + scrollContainer, + suppressAutoMark, + ]); return scrollSentinelRef; } @@ -284,9 +321,11 @@ function useScrollIntoViewOnSelect( isSelected: boolean | undefined, sectionRef: React.RefObject | undefined, setCollapsed: React.Dispatch>, + suppressAutoMark: React.RefObject, ) { useEffect(() => { if (isSelected) { + suppressAutoMark.current = true; setCollapsed(false); requestAnimationFrame(() => { requestAnimationFrame(() => { @@ -294,7 +333,7 @@ function useScrollIntoViewOnSelect( }); }); } - }, [isSelected, sectionRef, setCollapsed]); + }, [isSelected, sectionRef, setCollapsed, suppressAutoMark]); } /** @@ -352,6 +391,7 @@ function renderDiffContent(opts: { file: ReviewFile; sessionId: string; wordWrap: boolean; + enableWalkthroughAnnotations: boolean; expandUnchanged: boolean; enableExpansion: boolean; baseRef: string; @@ -364,6 +404,7 @@ function renderDiffContent(opts: { file, sessionId, wordWrap, + enableWalkthroughAnnotations, expandUnchanged, enableExpansion, baseRef, @@ -382,6 +423,7 @@ function renderDiffContent(opts: { status={file.status} enableComments enableAcceptReject + enableWalkthroughAnnotations={enableWalkthroughAnnotations} onRevertBlock={onRevertBlock} onCommentRun={onCommentRun} sessionId={sessionId} @@ -460,6 +502,34 @@ function getMarkdownPreviewToggle({ return () => onPreviewMarkdown(file.path, file.repository_name); } +type FileDiffActionsArgs = Pick< + FileDiffSectionProps, + "file" | "fileKey" | "sessionId" | "onToggleReviewed" | "onDiscard" +>; + +function useFileDiffActions({ + file, + fileKey, + sessionId, + onToggleReviewed, + onDiscard, +}: FileDiffActionsArgs) { + const handleCheckboxChange = useCallback( + (checked: boolean | "indeterminate") => { + onToggleReviewed(fileKey, checked === true); + }, + [fileKey, onToggleReviewed], + ); + const handleDiscard = useCallback(() => onDiscard(fileKey), [fileKey, onDiscard]); + const handleRevertBlock = useCallback( + (filePath: string, info: RevertBlockInfo) => + revertBlock(sessionId, filePath, info, file.repository_name), + [sessionId, file.repository_name], + ); + const handleCommentRun = useCommentRunHandler(sessionId); + return { handleCheckboxChange, handleDiscard, handleRevertBlock, handleCommentRun }; +} + function FileDiffSection({ file, fileKey, @@ -468,6 +538,7 @@ function FileDiffSection({ sessionId, autoMarkOnScroll, wordWrap, + enableWalkthroughAnnotations, isSelected, forceLoad, onToggleReviewed, @@ -476,6 +547,7 @@ function FileDiffSection({ onPreviewMarkdown, sectionRef, scrollContainer, + suppressAutoMark, externalLinkContext, }: FileDiffSectionProps) { const controls = useFileDiffDisplayControls(wordWrap); @@ -484,7 +556,7 @@ function FileDiffSection({ const { isVisible, sentinelRef } = useLazyVisible(scrollContainer); // Force load when visible via intersection observer, or forceLoad is true const shouldRenderContent = isVisible || !!forceLoad; - useScrollIntoViewOnSelect(isSelected, sectionRef, controls.setCollapsed); + useScrollIntoViewOnSelect(isSelected, sectionRef, controls.setCollapsed, suppressAutoMark); // Auto-mark sends the composite key (matches the dialog's reviewed-set // shape) so cross-repo same-named files don't all get marked when one // scrolls past. @@ -495,25 +567,10 @@ function FileDiffSection({ fileKey, onToggleReviewed, scrollContainer, + suppressAutoMark, }); - const handleCheckboxChange = useCallback( - (checked: boolean | "indeterminate") => { - onToggleReviewed(fileKey, checked === true); - }, - [fileKey, onToggleReviewed], - ); - const handleDiscard = useCallback(() => { - onDiscard(fileKey); - }, [fileKey, onDiscard]); - - const handleRevertBlock = useCallback( - (filePath: string, info: RevertBlockInfo) => - revertBlock(sessionId, filePath, info, file.repository_name), - [sessionId, file.repository_name], - ); - - const handleCommentRun = useCommentRunHandler(sessionId); - + const { handleCheckboxChange, handleDiscard, handleRevertBlock, handleCommentRun } = + useFileDiffActions({ file, fileKey, sessionId, onToggleReviewed, onDiscard }); const { enableExpansion, baseRef } = resolveDiffExpansion( file, externalLinkContext.baseBranchByRepo, @@ -557,6 +614,7 @@ function FileDiffSection({ file, sessionId, wordWrap: controls.effectiveWordWrap, + enableWalkthroughAnnotations, expandUnchanged: controls.expandUnchanged, enableExpansion, baseRef, diff --git a/apps/web/components/review/review-file-tree.test.tsx b/apps/web/components/review/review-file-tree.test.tsx index aa5d745981..82e66b151d 100644 --- a/apps/web/components/review/review-file-tree.test.tsx +++ b/apps/web/components/review/review-file-tree.test.tsx @@ -60,6 +60,34 @@ describe("ReviewFileTree status markers", () => { }); }); +describe("ReviewFileTree ordering", () => { + it("preserves review-list order across root files and directories", () => { + const files = [file({ path: "a-root.ts" }), file({ path: "z-dir/nested.ts" })]; + + renderTree(files); + + const renderedPaths = screen + .getAllByTestId("review-file-row") + .map((row) => row.dataset.filePath); + expect(renderedPaths).toEqual(files.map((entry) => entry.path)); + }); + + it("preserves review-list order when a directory is interleaved with a root file", () => { + const files = [ + file({ path: "src/a.ts" }), + file({ path: "README.md" }), + file({ path: "src/b.ts" }), + ]; + + renderTree(files); + + const renderedPaths = screen + .getAllByTestId("review-file-row") + .map((row) => row.dataset.filePath); + expect(renderedPaths).toEqual(files.map((entry) => entry.path)); + }); +}); + describe("ReviewFileTree", () => { it("renders one row per file", () => { renderTree([file({ path: "src/a.ts" }), file({ path: "src/b.ts" })]); diff --git a/apps/web/components/review/types.ts b/apps/web/components/review/types.ts index 414efb51b5..df8e4cde5c 100644 --- a/apps/web/components/review/types.ts +++ b/apps/web/components/review/types.ts @@ -256,8 +256,30 @@ function prefixPaths(node: FileTreeNode, prefix: string): FileTreeNode { }; } +function getOrAppendDirectorySegment( + parent: FileTreeNode, + name: string, + logicalPath: string, + segmentCounts: Map, +): FileTreeNode { + const previousChild = parent.children!.at(-1); + if (previousChild?.isDir && previousChild.name === name) return previousChild; + + const segment = segmentCounts.get(logicalPath) ?? 0; + segmentCounts.set(logicalPath, segment + 1); + const child: FileTreeNode = { + name, + path: segment === 0 ? logicalPath : `${logicalPath}${FILE_KEY_SEP}segment:${segment}`, + isDir: true, + children: [], + }; + parent.children!.push(child); + return child; +} + function buildFlatTree(files: ReviewFile[]): FileTreeNode[] { const root: FileTreeNode = { name: "", path: "", isDir: true, children: [] }; + const directorySegmentCounts = new Map(); for (const file of files) { const parts = file.path.split("/"); @@ -276,12 +298,7 @@ function buildFlatTree(files: ReviewFile[]): FileTreeNode[] { file, }); } else { - let child = current.children!.find((c) => c.isDir && c.name === part); - if (!child) { - child = { name: part, path: partPath, isDir: true, children: [] }; - current.children!.push(child); - } - current = child; + current = getOrAppendDirectorySegment(current, part, partPath, directorySegmentCounts); } } } @@ -312,21 +329,8 @@ function buildFlatTree(files: ReviewFile[]): FileTreeNode[] { const collapsed = collapse(root); - // Sort: directories first, then files, alphabetically - function sortTree(nodes: FileTreeNode[]): FileTreeNode[] { - return nodes - .sort((a, b) => { - if (a.isDir && !b.isDir) return -1; - if (!a.isDir && b.isDir) return 1; - return a.name.localeCompare(b.name); - }) - .map((node) => { - if (node.isDir && node.children) { - return { ...node, children: sortTree(node.children) }; - } - return node; - }); - } - - return sortTree(collapsed.children ?? []); + // Directory nodes represent contiguous input segments. If a directory + // reappears after another root entry, its unique segment node lets preorder + // traversal preserve the caller's exact file order. + return collapsed.children ?? []; } diff --git a/apps/web/components/task/task-changes-panel.tsx b/apps/web/components/task/task-changes-panel.tsx index 0bd365ced2..6f9b1977bb 100644 --- a/apps/web/components/task/task-changes-panel.tsx +++ b/apps/web/components/task/task-changes-panel.tsx @@ -481,6 +481,7 @@ function ChangesPanelContent({ sessionId={activeSessionId} autoMarkOnScroll={autoMarkOnScroll} wordWrap={wordWrap} + enableWalkthroughAnnotations selectedFile={selectedFile} onToggleReviewed={onToggleReviewed} onDiscard={onDiscard} diff --git a/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts b/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts index 57903f97b6..ffefe42a43 100644 --- a/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts +++ b/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts @@ -40,6 +40,7 @@ test.describe("Mobile code walkthrough", () => { testPage, apiClient, seedData, + prCapture, }) => { await seedWalkthroughTask(testPage, apiClient, seedData); @@ -63,6 +64,8 @@ test.describe("Mobile code walkthrough", () => { await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog"))); const reviewDialog = testPage.getByRole("dialog", { name: "Review Changes" }); await expect(reviewDialog).toBeVisible({ timeout: 15_000 }); + await expect(reviewDialog.locator('[data-walkthrough-active="true"]')).toHaveCount(0); + await expect(reviewDialog.getByText("0 of 3 files reviewed")).toBeVisible(); await expectWalkthroughBehindDialog(testPage, reviewDialog, [ { locator: card, name: "walkthrough window" }, { @@ -70,6 +73,9 @@ test.describe("Mobile code walkthrough", () => { name: "walkthrough launcher", }, ]); + await prCapture.screenshot("mobile-review-with-walkthrough", { + caption: "Mobile Review stays isolated from the task walkthrough", + }); }); test("requests a walkthrough from Changes and opens the generated bottom sheet", async ({ diff --git a/apps/web/e2e/tests/review/review-file-status.spec.ts b/apps/web/e2e/tests/review/review-file-status.spec.ts index ab4723cf52..00959441ef 100644 --- a/apps/web/e2e/tests/review/review-file-status.spec.ts +++ b/apps/web/e2e/tests/review/review-file-status.spec.ts @@ -5,6 +5,7 @@ import { REVIEW_SIDEBAR_LIMITS } from "../../../hooks/use-review-sidebar-resize" import path from "node:path"; const ADDED_PATH = "review-status-added.ts"; +const NESTED_PATH = "review-status-nested/nested.ts"; const MODIFIED_PATH = "review-status-modified.ts"; const DELETED_PATH = "review-status-deleted.ts"; const MOVED_FROM_PATH = "review-status-old-name.ts"; @@ -18,6 +19,7 @@ test.describe("Review file status", () => { apiClient, seedData, backend, + prCapture, }) => { await testPage.addInitScript(({ key, width }) => sessionStorage.setItem(key, width), { key: REVIEW_SIDEBAR_LIMITS.storageKey, @@ -89,16 +91,19 @@ test.describe("Review file status", () => { git.createFile(DELETED_PATH, "remove me\n"); git.stageAll(); git.commit("seed review status files"); - git.createFile(ADDED_PATH, "added\n"); + git.createFile(ADDED_PATH, "added line\n".repeat(80)); git.stageFile(ADDED_PATH); + git.createFile(NESTED_PATH, "nested\n"); + git.stageFile(NESTED_PATH); git.modifyFile(MODIFIED_PATH, "after\n"); git.deleteFile(DELETED_PATH); const changesTab = testPage.getByTestId("dockview-tab-changes"); await expect(changesTab).toBeVisible(); await changesTab.click(); - for (const filePath of [ADDED_PATH, MODIFIED_PATH, DELETED_PATH]) { - await expect(testPage.getByTestId(`file-row-${filePath}`)).toBeVisible({ timeout: 20_000 }); + for (const filePath of [ADDED_PATH, NESTED_PATH, MODIFIED_PATH, DELETED_PATH]) { + const rowTestId = `file-row-${filePath.replace(/[/\\]/g, "-")}`; + await expect(testPage.getByTestId(rowTestId)).toBeVisible({ timeout: 20_000 }); } await testPage @@ -111,6 +116,14 @@ test.describe("Review file status", () => { const sidebar = dialog.getByTestId("review-dialog-sidebar"); await expect(sidebar).toBeVisible(); + const sidebarOrder = await sidebar + .getByTestId("review-file-row") + .evaluateAll((rows) => rows.map((row) => (row as HTMLElement).dataset.filePath)); + const diffOrder = await dialog + .getByTestId("review-file-header") + .evaluateAll((headers) => headers.map((header) => (header as HTMLElement).dataset.filePath)); + expect(sidebarOrder).toEqual(diffOrder); + for (const [path, name] of [ [ADDED_PATH, "Added"], [MODIFIED_PATH, "Modified"], @@ -158,6 +171,28 @@ test.describe("Review file status", () => { expect(geometry.markerRight).toBeLessThanOrEqual(geometry.rowRight); expect(geometry.markerRight).toBeLessThanOrEqual(geometry.sidebarRight); + await expect(dialog.getByText("0 of 5 files reviewed")).toBeVisible(); + await sidebar + .locator(`[data-testid="review-file-row"][data-file-path="${NESTED_PATH}"]`) + .click(); + const reviewScroll = dialog.getByTestId("review-diff-scroll"); + await expect + .poll(() => reviewScroll.evaluate((element) => element.scrollTop)) + .toBeGreaterThan(0); + await testPage.waitForTimeout(600); + await expect(dialog.getByText("0 of 5 files reviewed")).toBeVisible(); + await prCapture.screenshot("review-ordered-safe-jump", { + caption: "Review keeps tree and diff order aligned without auto-reviewing a file jump", + }); + + await reviewScroll.evaluate((element) => { + element.scrollTop = 0; + }); + await expect.poll(() => reviewScroll.evaluate((element) => element.scrollTop)).toBe(0); + await reviewScroll.hover(); + await testPage.mouse.wheel(0, 500); + await expect(dialog.getByText(/[1-5] of 5 files reviewed/)).toBeVisible(); + await movedRow.click(); await expect( dialog.getByText(`Moved from ${MOVED_FROM_PATH}; no textual changes`), diff --git a/apps/web/e2e/tests/review/walkthrough.spec.ts b/apps/web/e2e/tests/review/walkthrough.spec.ts index 987dbdd6dd..e9a6719ac8 100644 --- a/apps/web/e2e/tests/review/walkthrough.spec.ts +++ b/apps/web/e2e/tests/review/walkthrough.spec.ts @@ -344,9 +344,19 @@ test.describe("Code walkthrough", () => { const card = await openWalkthrough(testPage); await expect(session.walkthroughEditorRange()).toBeVisible({ timeout: 15_000 }); + for (let step = 0; step < 3; step++) { + await card.getByTestId("walkthrough-next").click(); + } + await expect(card.getByTestId("walkthrough-step-header")).toContainText("Step 4 / 5"); + await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog"))); const reviewDialog = testPage.getByRole("dialog", { name: "Review Changes" }); await expect(reviewDialog).toBeVisible({ timeout: 15_000 }); + await expect(reviewDialog.locator('[data-walkthrough-active="true"]')).toHaveCount(0); + await expect(reviewDialog.getByText("0 of 3 files reviewed")).toBeVisible({ timeout: 15_000 }); + await testPage.waitForTimeout(600); + await expect(reviewDialog.getByTestId("review-diff-scroll")).toHaveJSProperty("scrollTop", 0); + await expect(reviewDialog.getByText("0 of 3 files reviewed")).toBeVisible(); await expectWalkthroughBehindDialog(testPage, reviewDialog, [ { locator: card, name: "walkthrough window" }, { locator: session.walkthroughLauncher().locator(".."), name: "walkthrough launcher" }, diff --git a/docs/public/sessions-and-review.md b/docs/public/sessions-and-review.md index ab34438452..789101a229 100644 --- a/docs/public/sessions-and-review.md +++ b/docs/public/sessions-and-review.md @@ -152,7 +152,7 @@ During review you can: - discard a file or revert a supported diff block after confirmation; - attach a pending comment to a changed line. -Reviewed state is stored per session. Kandev also stores the diff hash: if the file changes after you review it, the file becomes stale and unreviewed. By default, scrolling through a file marks it reviewed; use the review toolbar to disable **Auto-mark reviewed on scroll**. +Reviewed state is stored per session. Kandev also stores the diff hash: if the file changes after you review it, the file becomes stale and unreviewed. By default, manually scrolling past a file marks it reviewed; file-selection jumps in Review do not. Use the review toolbar to disable **Auto-mark reviewed on scroll**. Review does not embed walkthrough steps in its diff list; follow a saved walkthrough from its launcher and file editor. Pending inline comments are scoped to the current review session but persist only in that browser's `sessionStorage`; they are not synced to the backend or another browser. Select **Fix comments** to send the accumulated file, line, source, and comment context to the agent and close the review dialog. If the agent is busy, normal session queuing applies. The UI clears pending comments immediately after starting the fire-and-forget send; if that request later fails, it shows an error but does not restore them. Copy important feedback before sending. Reopen the current diff before sending old feedback: a valid line number can still refer to different code after a rewrite.