fix(web): make review ordering and navigation safe (#2067)

2026-07-30 20:07:36 +01:00
parent cfa875d9d6
commit d4e8f0a62a
18 changed files with 453 additions and 90 deletions
@@ -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,
@@ -84,6 +84,17 @@ describe("DiffViewer", () => {
    await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(renderCount));
  });

+  it("rerenders when walkthrough annotations are disabled", async () => {
+    const { rerender } = render(<DiffViewer data={data} enableWalkthroughAnnotations />);
+
+    await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(0));
+    const renderCount = fileDiffProps.length;
+
+    rerender(<DiffViewer data={data} enableWalkthroughAnnotations={false} />);
+
+    await waitFor(() => expect(fileDiffProps.length).toBeGreaterThan(renderCount));
+  });
+
  it("keeps simple hunk separators when full-content reparse rejects expansion", async () => {
    expansionState.canExpand = false;

@@ -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> | 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,
@@ -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}
@@ -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();
+  });
 });
@@ -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<AnnotationMetadata> | 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<Map<string, string>>;
  revertInfoRef: RefObject<Map<string, RevertBlockInfo>>;
 }) {
@@ -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,
  });
@@ -61,6 +61,7 @@ function ReviewDialogDiffContent({
          sessionId={sessionId}
          autoMarkOnScroll={state.autoMarkOnScroll}
          wordWrap={state.wordWrap}
+          enableWalkthroughAnnotations={false}
          onToggleReviewed={state.handleToggleReviewed}
          onDiscard={state.handleDiscard}
          onOpenFile={onOpenFile}
@@ -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<typeof useReviewDialogState>;

-/**
- * 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 (
    <ReviewDialogSurface
      open={open}
@@ -0,0 +1,206 @@
+import { act, cleanup, fireEvent, render } from "@testing-library/react";
+import { createRef, type ReactNode } from "react";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { TooltipProvider } from "@kandev/ui/tooltip";
+import type { ReviewFile } from "./types";
+
+type ObserverRecord = {
+  callback: IntersectionObserverCallback;
+  instance: IntersectionObserver;
+  options?: IntersectionObserverInit;
+  targets: Element[];
+};
+
+const observerRecords: ObserverRecord[] = [];
+const diffViewerProps: Array<{ enableWalkthroughAnnotations?: boolean; filePath: string }> = [];
+
+vi.mock("@/components/diff", () => ({
+  FileDiffViewer: (props: { enableWalkthroughAnnotations?: boolean; filePath: string }) => {
+    diffViewerProps.push(props);
+    return <div>{props.filePath}</div>;
+  },
+  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<HTMLDivElement>()]));
+    const view = render(
+      <TooltipProvider>
+        <ReviewDiffList
+          files={files}
+          reviewedFiles={new Set()}
+          staleFiles={new Set()}
+          sessionId="session-1"
+          autoMarkOnScroll
+          wordWrap={false}
+          selectedFile="b.ts"
+          enableWalkthroughAnnotations={false}
+          onToggleReviewed={onToggleReviewed}
+          onDiscard={() => undefined}
+          fileRefs={refs}
+        />
+      </TooltipProvider>,
+    );
+    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(
+      <TooltipProvider>
+        <ReviewDiffList
+          files={[entry]}
+          reviewedFiles={new Set()}
+          staleFiles={new Set()}
+          sessionId="session-1"
+          autoMarkOnScroll={false}
+          wordWrap={false}
+          selectedFile="a.ts"
+          enableWalkthroughAnnotations={false}
+          onToggleReviewed={() => undefined}
+          onDiscard={() => undefined}
+          fileRefs={new Map([[entry.path, createRef<HTMLDivElement>()]])}
+        />
+      </TooltipProvider>,
+    );
+
+    expect(diffViewerProps.at(-1)?.enableWalkthroughAnnotations).toBe(false);
+  });
+});
@@ -78,6 +78,7 @@ const baseProps = {
  sessionId: "sess",
  autoMarkOnScroll: false,
  wordWrap: false,
+  enableWalkthroughAnnotations: true,
  selectedFile: null,
  onToggleReviewed: () => undefined,
  onDiscard: () => undefined,
@@ -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<string>;
@@ -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<HTMLDivElement | null>(null);
+  const suppressAutoMarkRef = useRef(false);
+  const allowAutoMark = useCallback(() => {
+    suppressAutoMarkRef.current = false;
+  }, []);
+  const handlePointerDown = useCallback((event: React.PointerEvent<HTMLDivElement>) => {
+    if (event.target === event.currentTarget) suppressAutoMarkRef.current = false;
+  }, []);
+  const handleKeyDown = useCallback((event: React.KeyboardEvent<HTMLDivElement>) => {
+    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 (
-    <div ref={scrollContainerRef} className="overflow-y-auto h-full">
+    <div
+      ref={scrollContainerRef}
+      data-testid="review-diff-scroll"
+      className="overflow-y-auto h-full"
+      onWheelCapture={allowAutoMark}
+      onTouchMoveCapture={allowAutoMark}
+      onPointerDownCapture={handlePointerDown}
+      onKeyDownCapture={handleKeyDown}
+    >
      {groups.map((group) => (
        <div
          key={group.repositoryName || "__no_repo__"}
@@ -99,6 +121,7 @@ export const ReviewDiffList = memo(function ReviewDiffList({
                sessionId={sessionId}
                autoMarkOnScroll={autoMarkOnScroll}
                wordWrap={wordWrap}
+                enableWalkthroughAnnotations={enableWalkthroughAnnotations}
                isSelected={selectedFile === key}
                forceLoad={
                  selectedIndex >= 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<HTMLDivElement | null>;
  scrollContainer: React.RefObject<HTMLDivElement | null>;
+  suppressAutoMark: React.RefObject<boolean>;
  /** 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<HTMLDivElement | null>;
+  suppressAutoMark: React.RefObject<boolean>;
 };

 function useAutoMarkOnScroll({
@@ -191,6 +218,7 @@ function useAutoMarkOnScroll({
  fileKey,
  onToggleReviewed,
  scrollContainer,
+  suppressAutoMark,
 }: AutoMarkArgs) {
  const scrollSentinelRef = useRef<HTMLDivElement | null>(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<HTMLDivElement | null> | undefined,
  setCollapsed: React.Dispatch<React.SetStateAction<boolean>>,
+  suppressAutoMark: React.RefObject<boolean>,
 ) {
  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,
@@ -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" })]);
@@ -256,8 +256,30 @@ function prefixPaths(node: FileTreeNode, prefix: string): FileTreeNode {
  };
 }

+function getOrAppendDirectorySegment(
+  parent: FileTreeNode,
+  name: string,
+  logicalPath: string,
+  segmentCounts: Map<string, number>,
+): 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<string, number>();

  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 ?? [];
 }
@@ -481,6 +481,7 @@ function ChangesPanelContent({
      sessionId={activeSessionId}
      autoMarkOnScroll={autoMarkOnScroll}
      wordWrap={wordWrap}
+      enableWalkthroughAnnotations
      selectedFile={selectedFile}
      onToggleReviewed={onToggleReviewed}
      onDiscard={onDiscard}
@@ -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 ({
@@ -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`),
@@ -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" },
@@ -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.