fix(vcs): open Commit dialog diffs in a front window (#1875)

* fix(vcs): open Commit dialog diffs in a front window (#1863)

In the modal Commit dialog, activating a changed file opened its diff in an
editor tab behind the dialog, invisible until the dialog closed. Open it in a
front Dialog stacked above instead (IntelliJ's diff window): Esc, the close
button or the close-tab chord returns to the Commit dialog with its message,
selection and options intact.

The dialog reuses DiffEditor with an embedded DiffData, so the full viewer
toolbar (prev/next difference, split/unified, ignore whitespace, highlighting)
is there. Chain navigation (F7 / Shift+F7 past an edge, diff.nextFile /
prevFile) now goes through a new onNeighbour hook instead of openDiff: an
embedded host swaps its own content rather than opening a hidden tab. Only
one diff window is kept, as in IDEA.

* fix(vcs): open the Commit dialog menu's Show Diff in a front window

The dialog-mode context menu still used the global vcs.showDiff action, which
opens an editor tab behind the modal Commit dialog. Route it through the same
front diff dialog as double-click.

src/features/vcs/CommitView.tsx

@@ -70,6 +70,7 @@ import { pushAfterCommit } from "./commitUi";
 import { rebaseAfterFixup } from "./rebase";
 import { stashTabOf, stashTabs, type ContentTab } from "./gitTabs";
 import { diffInput, openDiff, withChain, workingDiffData } from "./diffInput";
+import { openDiffDialog } from "./diffDialog";
 import { DiffEditor } from "./DiffEditor";
 import { KIND_COLOR, KIND_LABEL, fileCount, formatTime, stashLabel, stripSlash } from "./model";
 import { call, commands } from "@/platform/ipc";
@@ -182,12 +183,18 @@ const bottomLeft = (el: HTMLElement) => {
   return { x: r.left, y: r.bottom + 2 };
 };
 
-/** Opens a file's diff; F7 continues through the other files of its group, in tree order. */
-function openNode(n: ChangeNode, roots: readonly ChangeNode[]) {
+/**
+ * Opens a file's diff; F7 continues through the other files of its group, in tree
+ * order. `dialog` opens it in a front dialog: an editor tab would sit behind the
+ * modal Commit dialog, invisible until it closes (#1863).
+ */
+function openNode(n: ChangeNode, roots: readonly ChangeNode[], mode: "commit" | "local" | "dialog") {
   if (n.type !== "file" || !n.path) return;
   const nodes = groupFileNodes(roots, n);
   const items = nodes.map((f) => workingDiffData(f.path!, undefined, stagingSide(f.group, f.files[0]!)));
-  void openDiff(withChain(items, Math.max(0, nodes.indexOf(n))), { preview: true });
+  const data = withChain(items, Math.max(0, nodes.indexOf(n)));
+  if (mode === "dialog") openDiffDialog(data);
+  else void openDiff(data, { preview: true });
 }
 
 /** Context menu of a change list's header: the list's own actions, then the bulk ones on its files. */
@@ -239,7 +246,11 @@ export function deletableFiles(nodes: ChangeNode[]): string[] {
   return files.length && files.every(onDisk) ? files.map((f) => f.path) : [];
 }
 
-export function nodeMenu(nodes: ChangeNode[], repos: readonly string[]): MenuItem[] {
+export function nodeMenu(
+  nodes: ChangeNode[],
+  repos: readonly string[],
+  opts?: { mode?: "commit" | "local" | "dialog"; roots?: readonly ChangeNode[] },
+): MenuItem[] {
   const files = filesOf(nodes);
   const deletable = deletableFiles(nodes);
   const paths = files.map((f) => f.path);
@@ -268,7 +279,11 @@ export function nodeMenu(nodes: ChangeNode[], repos: readonly string[]): MenuIte
       run: () => commitFiles(paths),
     },
     { kind: "separator" },
-    { kind: "action", action: "vcs.showDiff", arg: first?.path },
+    // In `dialog` mode an editor tab would sit behind the modal Commit dialog (#1863):
+    // open the diff in a front dialog, like double-click does.
+    opts?.mode === "dialog" && opts.roots && first
+      ? { label: "Show Diff", icon: "diff", run: () => openNode(first, opts.roots!, "dialog") }
+      : { kind: "action", action: "vcs.showDiff", arg: first?.path },
     {
       label: "Jump to Source",
       icon: "external-link",
@@ -645,7 +660,7 @@ export function ChangesTree({ mode }: { mode: "commit" | "local" | "dialog" }) {
               selectedList: nodes.find((n) => n.changelist)?.changelist ?? null,
             });
           }}
-          onActivate={(n) => openNode(n, roots)}
+          onActivate={(n) => openNode(n, roots, mode)}
           onContextMenu={(node, sel, e: MouseEvent) => {
             if (!node) return;
             if (node.type === "group" && node.changelist) {
@@ -653,7 +668,7 @@ export function ChangesTree({ mode }: { mode: "commit" | "local" | "dialog" }) {
               return;
             }
             const nodes = [...sel].map((k) => byKey.get(k)).filter((n): n is ChangeNode => !!n);
-            showContextMenu(e, nodeMenu(nodes.length ? nodes : [node], repos));
+            showContextMenu(e, nodeMenu(nodes.length ? nodes : [node], repos, { mode, roots }));
           }}
           aria-label="Changes"
           empty={

src/features/vcs/DiffEditor.tsx

@@ -211,7 +211,20 @@ function isStrokeOf(e: KeyboardEvent, actionId: string): boolean {
   return !!stroke && bindingsFor(actionId).some((b) => normalizeBinding(b) === stroke);
 }
 
-export function DiffEditor({ input, toolbarActions }: { input: EditorInput; toolbarActions?: ReactNode }) {
+export function DiffEditor({
+  input,
+  toolbarActions,
+  onNeighbour,
+}: {
+  input: EditorInput;
+  toolbarActions?: ReactNode;
+  /**
+   * Called to show another file of the chain instead of `openDiff` (which opens an
+   * editor tab): embedded hosts such as the Commit dialog's diff window swap their
+   * own content, as a modal dialog hides the editor area.
+   */
+  onNeighbour?: (data: DiffData) => void;
+}) {
   const data = input.data as DiffData;
   const { path, left, right } = data;
   const [attached, setAttached] = useState<Attached | null>(null);
@@ -595,12 +608,14 @@ export function DiffEditor({ input, toolbarActions }: { input: EditorInput; tool
       navigation.current = request;
       try {
         const neighbour = await changedChainNeighbour(data, dir, opts.ignore);
-        if (navigation.current === request && neighbour) await openDiff(neighbour, { preview: true });
+        if (navigation.current !== request || !neighbour) return;
+        if (onNeighbour) onNeighbour(neighbour);
+        else await openDiff(neighbour, { preview: true });
       } finally {
         if (navigation.current === request) navigation.current = null;
       }
     },
-    [data, opts.ignore],
+    [data, opts.ignore, onNeighbour],
   );
   useEffect(() => {
     if (!navHint) return;

src/features/vcs/__tests__/diffDialog.test.tsx

@@ -0,0 +1,114 @@
+import { act, fireEvent, render, screen } from "@testing-library/react";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+vi.mock("@/platform/ipc", async (importOriginal) => {
+  const real = await importOriginal<typeof import("@/platform/ipc")>();
+  return { ...real, call: vi.fn() };
+});
+
+const editor = vi.hoisted(() => vi.fn());
+vi.mock("../DiffEditor", () => ({
+  DiffEditor: (props: { input: { data: { path: string; embedded?: boolean } } }) => {
+    editor(props);
+    return <div data-testid="diff-view" data-path={props.input.data.path} />;
+  },
+}));
+
+const openDiff = vi.hoisted(() => vi.fn());
+vi.mock("../diffInput", async (original) => ({
+  ...(await original<typeof import("../diffInput")>()),
+  openDiff,
+}));
+
+import { ChangesTree, nodeMenu } from "../CommitView";
+import type { ChangeNode } from "../changes";
+import { VcsDialogHost } from "../dialogs";
+import { useVcs } from "../store";
+
+const modified = (path: string) => ({
+  path,
+  relPath: path.slice(3),
+  staged: null,
+  unstaged: "modified" as const,
+  renamedFrom: null,
+});
+
+/** Activates (double-clicks) the file row the way a user does. */
+function activateFile(name: string) {
+  const row = screen.getByText(name).closest('[role="treeitem"]')!;
+  act(() => fireEvent.doubleClick(row));
+}
+
+/** The diff window of this test: named by the file's diff title (the tree may double-activate). */
+const diffDialogs = async () => (await screen.findAllByRole("dialog", { name: /\(HEAD ↔ Your version\)/ })).at(-1)!;
+
+describe("Commit dialog diff window (#1863)", () => {
+  beforeEach(() => {
+    openDiff.mockClear();
+    editor.mockClear();
+    // The dialog stack is module state: close whatever a previous test left open.
+    for (const close of screen.queryAllByRole("button", { name: "Close" })) act(() => fireEvent.click(close));
+    useVcs.setState({ available: true, root: "/p", repo: "/p", roots: ["/p"], status: [modified("/p/a.ts")] });
+  });
+
+  it("opens the diff in a front dialog, not in an editor tab", async () => {
+    render(
+      <>
+        <ChangesTree mode="dialog" />
+        <VcsDialogHost />
+      </>,
+    );
+    activateFile("a.ts");
+    expect(openDiff).not.toHaveBeenCalled();
+    await diffDialogs();
+    expect(screen.getByTestId("diff-view")).toHaveAttribute("data-path", "/p/a.ts");
+    expect(editor.mock.calls.at(-1)?.[0].input.data.embedded).toBe(true);
+  });
+
+  it("keeps the changes tree mounted underneath and closes on Esc", async () => {
+    render(
+      <>
+        <ChangesTree mode="dialog" />
+        <VcsDialogHost />
+      </>,
+    );
+    activateFile("a.ts");
+    await diffDialogs();
+    expect(screen.getByRole("tree", { name: "Changes" })).toBeTruthy();
+    act(() => fireEvent.keyDown(document, { key: "Escape" }));
+    expect(screen.queryByRole("dialog", { name: /\(HEAD ↔ Your version\)/ })).toBeNull();
+    // Back at the Commit dialog's tree, unchanged.
+    expect(screen.getByRole("tree", { name: "Changes" })).toBeTruthy();
+  });
+
+  it("opens the context menu's Show Diff in a front dialog, not in an editor tab", async () => {
+    render(<VcsDialogHost />);
+    const f = modified("/p/a.ts");
+    const node: ChangeNode = { key: f.path, type: "file", label: "a.ts", group: "changes", files: [f], path: f.path };
+    const item = nodeMenu([node], ["/p"], { mode: "dialog", roots: [node] }).find(
+      (i) => i.kind !== "separator" && "label" in i && i.label === "Show Diff",
+    );
+    expect(item).toMatchObject({ label: "Show Diff" });
+    act(() => (item as { run: () => void }).run());
+    expect(openDiff).not.toHaveBeenCalled();
+    await diffDialogs();
+    expect(screen.getByTestId("diff-view")).toHaveAttribute("data-path", "/p/a.ts");
+  });
+
+  it("moves along the chain inside the dialog instead of opening editor tabs", async () => {
+    useVcs.setState({ status: [modified("/p/a.ts"), modified("/p/b.ts")] });
+    render(
+      <>
+        <ChangesTree mode="dialog" />
+        <VcsDialogHost />
+      </>,
+    );
+    activateFile("a.ts");
+    await diffDialogs();
+    const neighbour = { path: "/p/b.ts", left: { type: "rev", rev: "HEAD" }, right: { type: "worktree" } };
+    act(() => editor.mock.calls.at(-1)?.[0].onNeighbour(neighbour));
+    expect(openDiff).not.toHaveBeenCalled();
+    expect(screen.getByTestId("diff-view")).toHaveAttribute("data-path", "/p/b.ts");
+    await screen.findByRole("dialog", { name: "b.ts (HEAD ↔ Your version)" });
+  });
+});

src/features/vcs/diffDialog.tsx

@@ -0,0 +1,79 @@
+/**
+ * The diff "window" of the modal Commit dialog (#1863): activating a changed file
+ * there opens the diff in front of the dialog, not in an editor tab behind it.
+ * A `Dialog` stacked on `openDialog`'s stack keeps the Commit dialog (and its
+ * message, selection and options) mounted underneath; Esc / the close button /
+ * the close-tab chord return to it. F7 / Shift+F7 and the diff navigation actions
+ * move along the file's change chain by swapping this dialog's content.
+ */
+import { useState, type KeyboardEvent } from "react";
+import { bindingsFor, normalizeBinding, strokeOf } from "@/platform/keymap";
+import { Dialog } from "@/ui";
+import { DiffEditor } from "./DiffEditor";
+import { diffInput, diffTitle, type DiffData } from "./diffInput";
+import { openDialog } from "./dialogs";
+
+/** Whether `e` is one of the strokes bound to `actionId`. */
+function isStrokeOf(e: KeyboardEvent, actionId: string): boolean {
+  const stroke = strokeOf(e.nativeEvent);
+  return !!stroke && bindingsFor(actionId).some((b) => normalizeBinding(b) === stroke);
+}
+
+/** The open diff dialog's close function (one diff window at a time, as in IDEA). */
+let shown: (() => void) | null = null;
+
+/**
+ * Opens `data` in a front dialog; returns its close function. A second activation
+ * replaces the open diff window instead of stacking another on top.
+ */
+export function openDiffDialog(data: DiffData): () => void {
+  shown?.();
+  shown = null;
+  const close = openDialog((c) => (
+    <DiffDialog
+      data={data}
+      onClose={() => {
+        if (shown === close) shown = null;
+        c();
+      }}
+    />
+  ));
+  shown = close;
+  return () => {
+    if (shown === close) shown = null;
+    close();
+  };
+}
+
+function DiffDialog({ data: initial, onClose }: { data: DiffData; onClose: () => void }) {
+  const [data, setData] = useState(initial);
+  const input = diffInput({ ...data, embedded: true });
+  return (
+    <Dialog
+      open
+      title={diffTitle(data)}
+      onClose={onClose}
+      width="calc(100vw - 64px)"
+      resizable="both"
+      minWidth={480}
+      minHeight={280}
+      initialSize={{ w: Math.min(1100, globalThis.innerWidth - 64), h: Math.min(720, globalThis.innerHeight - 64) }}
+      buttons={[{ label: "Close", role: "cancel" }]}
+    >
+      <div
+        className="vcsDiffDialogBody"
+        style={{ display: "flex", flexDirection: "column", flex: 1, minHeight: 0 }}
+        onKeyDown={(e) => {
+          // The "close tab" chord would otherwise close an editor tab behind the modal.
+          if (isStrokeOf(e, "toolwindow.closeContent")) {
+            e.preventDefault();
+            e.stopPropagation();
+            onClose();
+          }
+        }}
+      >
+        <DiffEditor key={input.key} input={input} onNeighbour={setData} />
+      </div>
+    </Dialog>
+  );
+}