From 34d830c24723aa26fc5bbcaa8b2662016f636971 Mon Sep 17 00:00:00 2001 From: arookieofc Date: Sat, 7 Nov 2026 03:17:13 +0800 Subject: [PATCH] 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 | 29 +++-- src/features/vcs/DiffEditor.tsx | 21 +++- .../vcs/__tests__/diffDialog.test.tsx | 114 ++++++++++++++++++ src/features/vcs/diffDialog.tsx | 79 ++++++++++++ 4 files changed, 233 insertions(+), 10 deletions(-) create mode 100644 src/features/vcs/__tests__/diffDialog.test.tsx create mode 100644 src/features/vcs/diffDialog.tsx diff --git a/src/features/vcs/CommitView.tsx b/src/features/vcs/CommitView.tsx index 1329403f2..04d296747 100644 --- a/src/features/vcs/CommitView.tsx +++ b/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={ diff --git a/src/features/vcs/DiffEditor.tsx b/src/features/vcs/DiffEditor.tsx index 89089b932..32a3c9013 100644 --- a/src/features/vcs/DiffEditor.tsx +++ b/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(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; diff --git a/src/features/vcs/__tests__/diffDialog.test.tsx b/src/features/vcs/__tests__/diffDialog.test.tsx new file mode 100644 index 000000000..a7ef66887 --- /dev/null +++ b/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(); + 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
; + }, +})); + +const openDiff = vi.hoisted(() => vi.fn()); +vi.mock("../diffInput", async (original) => ({ + ...(await original()), + 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( + <> + + + , + ); + 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( + <> + + + , + ); + 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(); + 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( + <> + + + , + ); + 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)" }); + }); +}); diff --git a/src/features/vcs/diffDialog.tsx b/src/features/vcs/diffDialog.tsx new file mode 100644 index 000000000..73056a84a --- /dev/null +++ b/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) => ( + { + 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 ( + +
{ + // The "close tab" chord would otherwise close an editor tab behind the modal. + if (isStrokeOf(e, "toolwindow.closeContent")) { + e.preventDefault(); + e.stopPropagation(); + onClose(); + } + }} + > + +
+ + ); +}