From d5e77478036de77a524690c708906c2065206aca Mon Sep 17 00:00:00 2001 From: zhangdagou Date: Sun, 8 Nov 2026 21:16:34 +0800 Subject: [PATCH] =?UTF-8?q?fix(vcs):=20=E4=BF=AE=E5=A4=8D=20Git=20?= =?UTF-8?q?=E5=AF=B9=E6=AF=94=E5=AE=9A=E4=BD=8D=E4=B8=8E=E9=80=89=E5=8C=BA?= =?UTF-8?q?=E5=BC=82=E5=B8=B8=EF=BC=8C=E4=BC=98=E5=8C=96=E8=B7=B3=E8=BD=AC?= =?UTF-8?q?=E5=8F=8D=E9=A6=88=20(#2093)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching the compared file did not select it in the changes tree, a Git status refresh reset the tree's selection and collapsed folders, chunk jumps moved abruptly, and a single click after a jump could select several lines because WebKit scrolls to the old DOM selection on focus. The tree now follows the active diff without taking focus and keeps its state across refreshes. Next and previous chunk scroll smoothly with a short fade on entering a diff, both honouring reduced motion. The caret is placed before focusing on a click. Sibling covered: delayed scroll events from the synchronized pane no longer interrupt a smooth jump. Co-authored-by: PLZY <41029728+PLZY@users.noreply.github.com> --- src/features/vcs/CommitView.tsx | 65 +++++++++-- src/features/vcs/DiffEditor.module.css | 18 +++ src/features/vcs/DiffEditor.tsx | 44 +++++++- .../vcs/__tests__/commitPreviewDiff.test.tsx | 105 ++++++++++++++++++ src/features/vcs/__tests__/diffScroll.test.ts | 80 ++++++++++++- src/features/vcs/diffScroll.ts | 43 ++++++- 6 files changed, 341 insertions(+), 14 deletions(-) diff --git a/src/features/vcs/CommitView.tsx b/src/features/vcs/CommitView.tsx index e6498a6f8..156c8124b 100644 --- a/src/features/vcs/CommitView.tsx +++ b/src/features/vcs/CommitView.tsx @@ -4,7 +4,16 @@ */ import { useWorkflowSettings } from "@/platform/workflowSettings"; import { formatBinding, isMacHost, macStroke, normalizeBinding, strokeOf } from "@/platform/keymap"; -import { useCallback, useEffect, useMemo, useRef, useState, type MouseEvent, type PointerEvent } from "react"; +import { + useCallback, + useEffect, + useLayoutEffect, + useMemo, + useRef, + useState, + type MouseEvent, + type PointerEvent, +} from "react"; import { tx } from "@/platform/i18n"; import clsx from "clsx"; import { executeAction } from "@/platform/actions"; @@ -33,7 +42,7 @@ import { type TreeHandle, T, } from "@/ui"; -import { editorService, useMenuSlotItems } from "@/workbench/contributions"; +import { editorService, maybeEditorService, type EditorInput, useMenuSlotItems } from "@/workbench/contributions"; import { evaluateWhen, useContextKeys } from "@/platform/context"; import { useToolWindowLayout } from "@/workbench/toolwindows/store"; import { @@ -70,7 +79,7 @@ import { import { pushAfterCommit } from "./commitUi"; import { rebaseAfterFixup } from "./rebase"; import { stashTabOf, stashTabs, type ContentTab } from "./gitTabs"; -import { diffInput, openDiff, withChain, workingDiffData } from "./diffInput"; +import { DIFF_KIND, 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"; @@ -527,15 +536,51 @@ export function ChangesTree({ mode }: { mode: "commit" | "local" | "dialog" }) { return [m, up] as const; }, [roots]); - /** Select Opened File: reveals the focused editor's file in the tree. */ + const [nodeToReveal, setNodeToReveal] = useState<{ key: string } | null>(null); + const revealNode = useCallback( + (node: ChangeNode) => { + const ancestors = new Set(); + for (let key = parents.get(node.key); key; key = parents.get(key)) ancestors.add(key); + setCollapsed((current) => new Set([...current].filter((key) => !ancestors.has(key)))); + setNodeToReveal({ key: node.key }); + }, + [parents], + ); + + const followedDiff = useRef(null); + useEffect(() => { + const editor = maybeEditorService(); + const followDiff = (input: EditorInput | null) => { + if (input?.kind !== DIFF_KIND) { + followedDiff.current = null; + return; + } + if (input.key === followedDiff.current) return; + const node = [...byKey.values()].find((node) => { + if (node.type !== "file" || node.path !== input.path) return false; + const side = stagingSide(node.group, node.files[0]!); + return diffInput(workingDiffData(node.path!, undefined, side)).key === input.key; + }); + if (node) { + followedDiff.current = input.key; + revealNode(node); + } else { + followedDiff.current = null; + } + }; + followDiff(editor?.activeInput?.() ?? null); + return editor?.onActiveInputChange?.(followDiff); + }, [byKey, revealNode]); + + // Reveal only after the ancestors have expanded, without moving keyboard focus. + useLayoutEffect(() => { + if (nodeToReveal) treeRef.current?.reveal(nodeToReveal.key); + }, [nodeToReveal]); + const selectOpened = () => { const path = editorService().activePath(); - const node = path ? [...byKey.values()].find((n) => n.type === "file" && n.path === path) : undefined; - if (!node) return; - const ancestors: string[] = []; - for (let k = parents.get(node.key); k; k = parents.get(k)) ancestors.push(k); - setCollapsed((c) => new Set([...c].filter((k) => !ancestors.includes(k)))); - requestAnimationFrame(() => treeRef.current?.reveal(node.key)); + const node = path ? [...byKey.values()].find((node) => node.type === "file" && node.path === path) : undefined; + if (node) revealNode(node); }; const expanded = useMemo(() => { const s = new Set(); diff --git a/src/features/vcs/DiffEditor.module.css b/src/features/vcs/DiffEditor.module.css index cb0ba6729..041e32327 100644 --- a/src/features/vcs/DiffEditor.module.css +++ b/src/features/vcs/DiffEditor.module.css @@ -118,6 +118,9 @@ flex-direction: column; overflow: hidden; } +.host:not([hidden]) { + animation: diff-enter var(--dur) var(--ease) both; +} .host[hidden] { display: none; } @@ -346,3 +349,18 @@ font-size: var(--fs-small); pointer-events: none; } + +@keyframes diff-enter { + from { + opacity: 0; + } + to { + opacity: 1; + } +} + +@media (prefers-reduced-motion: reduce) { + .host:not([hidden]) { + animation: none; + } +} diff --git a/src/features/vcs/DiffEditor.tsx b/src/features/vcs/DiffEditor.tsx index 32a3c9013..2ac253588 100644 --- a/src/features/vcs/DiffEditor.tsx +++ b/src/features/vcs/DiffEditor.tsx @@ -385,6 +385,26 @@ export function DiffEditor({ focus: () => { focusedSide.current = side; }, + mousedown: (event, view) => { + if ( + view.hasFocus || + event.button !== 0 || + event.detail !== 1 || + event.shiftKey || + event.altKey || + event.metaKey || + event.ctrlKey || + !view.contentDOM.contains(event.target as Node) + ) + return false; + const pos = view.posAtCoords({ x: event.clientX, y: event.clientY }); + if (pos === null) return false; + // WebKit may scroll to the old DOM selection on focus. Place the caret first so + // the default mouse handler measures the same position before and after focusing. + view.dispatch({ selection: { anchor: pos }, userEvent: "select.pointer" }); + view.focus(); + return false; + }, }); const collapseUnchanged = opts.collapse ? { margin: 3, minSize: 6 } : undefined; const diffConfig = ideaDiffConfig(opts.ignore, opts.highlight); @@ -649,7 +669,29 @@ export function DiffEditor({ } pendingFile.current = 0; setNavHint(null); - (dir === 1 ? goToNextChunk : goToPreviousChunk)({ state: v.state, dispatch: (tr) => v.dispatch(tr) }); + (dir === 1 ? goToNextChunk : goToPreviousChunk)({ + state: v.state, + dispatch: (tr) => { + v.dispatch({ selection: tr.selection, userEvent: "select.byChunk" }); + const scroller = views.merge && !views.panes ? views.merge.dom : v.scrollDOM; + v.requestMeasure({ + key: step, + read: () => { + const documentTop = v.documentTop - scroller.getBoundingClientRect().top + scroller.scrollTop; + const lineTop = v.lineBlockAt(v.state.selection.main.head).top; + return documentTop + lineTop - scroller.clientHeight / 3; + }, + write: (top) => { + const options: ScrollToOptions = { + top: Math.max(0, top), + behavior: window.matchMedia("(prefers-reduced-motion: reduce)").matches ? "instant" : "smooth", + }; + if (views.panes) views.panes.scrollTo(onA ? "a" : "b", options); + else scroller.scrollTo(options); + }, + }); + }, + }); if (v !== views.b) { // Mirror the caret into the right pane so the counter follows. const chunks = getChunks(v.state)?.chunks ?? []; diff --git a/src/features/vcs/__tests__/commitPreviewDiff.test.tsx b/src/features/vcs/__tests__/commitPreviewDiff.test.tsx index 6820d4573..02f58e848 100644 --- a/src/features/vcs/__tests__/commitPreviewDiff.test.tsx +++ b/src/features/vcs/__tests__/commitPreviewDiff.test.tsx @@ -1,5 +1,28 @@ import { act, fireEvent, render, screen } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { EditorInput } from "@/workbench/contributions"; + +const editor = vi.hoisted(() => ({ + input: null as EditorInput | null, + follow: null as ((input: EditorInput | null) => void) | null, +})); +beforeEach(() => { + editor.input = null; + editor.follow = null; +}); + +vi.mock("@/workbench/contributions", async (original) => ({ + ...(await original()), + maybeEditorService: () => ({ + activeInput: () => editor.input, + onActiveInputChange: (follow: typeof editor.follow) => { + editor.follow = follow; + return () => { + editor.follow = null; + }; + }, + }), +})); vi.mock("@/platform/ipc", async (importOriginal) => { const real = await importOriginal(); @@ -20,6 +43,7 @@ vi.mock("../diffInput", async (original) => ({ import { CommitToolWindow } from "../CommitView"; import { setCommitDiffPreview, useCommit } from "../commitStore"; +import { diffInput, workingDiffData } from "../diffInput"; import { useVcs } from "../store"; /** The splitter between the changes and the preview: the only vertical one in the layout. */ @@ -141,3 +165,84 @@ describe("Commit tool window Preview Diff (#1599)", () => { expect(screen.queryByTestId("diff-preview")).toBeNull(); }); }); + +it("follows the active working diff without following other revisions or taking focus", () => { + useCommit.setState({ commitDiffPreview: false, selection: [] }); + useVcs.setState({ + available: true, + root: "/p", + repo: "/p", + roots: ["/p"], + status: [modified("/p/a.ts"), modified("/p/b.ts")], + }); + render(); + const message = screen.getByPlaceholderText("Commit Message"); + message.focus(); + act(() => editor.follow?.(diffInput(workingDiffData("/p/b.ts")))); + expect(useCommit.getState().selection).toEqual(["/p/b.ts"]); + expect(screen.getByRole("treeitem", { name: /b.ts/ })).toHaveAttribute("aria-selected", "true"); + expect(document.activeElement).toBe(message); + act(() => + editor.follow?.( + diffInput({ path: "/p/a.ts", left: { type: "rev", rev: "old" }, right: { type: "rev", rev: "new" } }), + ), + ); + expect(useCommit.getState().selection).toEqual(["/p/b.ts"]); +}); + +describe("following the active diff across Git status refreshes", () => { + beforeEach(() => { + editor.input = diffInput(workingDiffData("/p/b.ts")); + useCommit.setState({ commitDiffPreview: false, selection: [], groupByDir: false, groupByStaging: false }); + useVcs.setState({ + available: true, + root: "/p", + repo: "/p", + roots: ["/p"], + status: [modified("/p/a.ts"), modified("/p/b.ts")], + }); + }); + + it("keeps a manually selected file when status refreshes with the same active diff", () => { + render(); + expect(useCommit.getState().selection).toEqual(["/p/b.ts"]); + selectFirstFile(); + expect(useCommit.getState().selection).toEqual(["/p/a.ts"]); + act(() => useVcs.setState({ status: [modified("/p/a.ts"), modified("/p/b.ts")] })); + expect(useCommit.getState().selection).toEqual(["/p/a.ts"]); + act(() => editor.follow?.(editor.input)); + expect(useCommit.getState().selection).toEqual(["/p/a.ts"]); + }); + + it("keeps a manually collapsed group across status refreshes", () => { + render(); + const tree = screen.getByRole("tree", { name: "Changes" }); + tree.focus(); + fireEvent.keyDown(tree, { key: "Home" }); + fireEvent.keyDown(tree, { key: "ArrowLeft" }); + expect(tree.querySelector('[role="treeitem"][aria-expanded="false"]')).toBeTruthy(); + expect(screen.queryByRole("treeitem", { name: /b.ts/ })).toBeNull(); + act(() => useVcs.setState({ status: [modified("/p/a.ts"), modified("/p/b.ts")] })); + expect(screen.queryByRole("treeitem", { name: /b.ts/ })).toBeNull(); + }); + + it.each([ + null, + diffInput({ path: "/p/b.ts", left: { type: "rev", rev: "old" }, right: { type: "rev", rev: "new" } }), + ])("follows the same diff again after switching away from it (%#)", (other) => { + render(); + selectFirstFile(); + act(() => editor.follow?.(other)); + act(() => editor.follow?.(editor.input)); + expect(useCommit.getState().selection).toEqual(["/p/b.ts"]); + }); +}); + +it("follows a newly opened diff when its Git status arrives later", () => { + editor.input = diffInput(workingDiffData("/p/new.ts")); + useCommit.setState({ commitDiffPreview: false, selection: [] }); + useVcs.setState({ available: true, root: "/p", repo: "/p", roots: ["/p"], status: [] }); + render(); + act(() => useVcs.setState({ status: [modified("/p/new.ts")] })); + expect(useCommit.getState().selection).toEqual(["/p/new.ts"]); +}); diff --git a/src/features/vcs/__tests__/diffScroll.test.ts b/src/features/vcs/__tests__/diffScroll.test.ts index 1668509f8..30bf85a9a 100644 --- a/src/features/vcs/__tests__/diffScroll.test.ts +++ b/src/features/vcs/__tests__/diffScroll.test.ts @@ -1,7 +1,8 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { Chunk, MergeView } from "@codemirror/merge"; import { Text } from "@codemirror/state"; -import { bandAt, bandPath, transferY, type Band } from "../diffScroll"; +import type { EditorView } from "@codemirror/view"; +import { bandAt, bandPath, transferY, UnalignedPanes, type Band } from "../diffScroll"; const doc = (s: string) => Text.of(s.split("\n")); const LINE = 20; @@ -95,3 +96,78 @@ describe("unaligned side-by-side scrolling", () => { expect(typeof (MergeView.prototype as unknown as { measure: unknown }).measure).toBe("function"); }); }); + +describe("animated synchronized scrolling", () => { + function panes(sync = true) { + const root = document.createElement("div"); + const text = doc(Array.from({ length: 200 }, () => "line").join("\n")); + const view = () => { + const wrapper = root.appendChild(document.createElement("div")); + const dom = wrapper.appendChild(document.createElement("div")); + const scrollDOM = dom.appendChild(document.createElement("div")); + Object.defineProperty(scrollDOM, "scrollHeight", { value: 4000 }); + scrollDOM.scrollTo = vi.fn(); + return { + dom, + scrollDOM, + state: { doc: text }, + get documentTop() { + return -scrollDOM.scrollTop; + }, + lineBlockAt: (pos: number) => ({ bottom: text.lineAt(pos).number * LINE }), + } as unknown as EditorView; + }; + const a = view(); + const b = view(); + const controller = new UnalignedPanes({ sync }); + controller.attach({ a, b, chunks: [] } as unknown as MergeView); + const scroll = (view: EditorView, top: number) => { + view.scrollDOM.scrollTop = top; + view.scrollDOM.dispatchEvent(new Event("scroll")); + }; + return { a, b, controller, scroll }; + } + + it.each(["a", "b"] as const)("ignores delayed follower layout scrolls while %s animates", (side) => { + vi.useFakeTimers(); + const frame = vi.spyOn(window, "requestAnimationFrame").mockReturnValue(1); + const p = panes(); + try { + const leader = p[side]; + const follower = p[side === "a" ? "b" : "a"]; + p.controller.scrollTo(side, { top: 2000, behavior: "smooth" }); + p.scroll(leader, 1000); + expect(follower.scrollDOM.scrollTop).toBe(1000); + p.scroll(follower, 200); + expect(leader.scrollDOM.scrollTop).toBe(1000); + expect(follower.scrollDOM.scrollTop).toBe(1000); + p.scroll(leader, 2000); + expect(follower.scrollDOM.scrollTop).toBe(2000); + vi.advanceTimersByTime(150); + p.scroll(follower, 300); + expect(leader.scrollDOM.scrollTop).toBe(300); + } finally { + p.controller.destroy(); + frame.mockRestore(); + vi.useRealTimers(); + } + }); + + it.each(["wheel", "pointerdown"])("lets %s interrupt an animated jump", (event) => { + vi.useFakeTimers(); + const frame = vi.spyOn(window, "requestAnimationFrame").mockReturnValue(1); + const p = panes(); + try { + p.controller.scrollTo("a", { top: 2000, behavior: "smooth" }); + p.scroll(p.a, 1000); + p.b.scrollDOM.dispatchEvent(new Event(event)); + expect(p.a.scrollDOM.scrollTo).toHaveBeenLastCalledWith({ top: 1000, behavior: "instant" }); + p.scroll(p.b, 400); + expect(p.a.scrollDOM.scrollTop).toBe(400); + } finally { + p.controller.destroy(); + frame.mockRestore(); + vi.useRealTimers(); + } + }); +}); diff --git a/src/features/vcs/diffScroll.ts b/src/features/vcs/diffScroll.ts index 476663016..7afcf536b 100644 --- a/src/features/vcs/diffScroll.ts +++ b/src/features/vcs/diffScroll.ts @@ -165,6 +165,8 @@ export class UnalignedPanes { b: { top: null, left: null }, }; private sync: boolean; + private scrolling: Side | null = null; + private scrollTimer: ReturnType | undefined; private frame = -1; private resize: ResizeObserver | null = null; private readonly cleanup: (() => void)[] = []; @@ -187,12 +189,20 @@ export class UnalignedPanes { for (const side of ["a", "b"] as const) { const el = this.view(side).scrollDOM; const onScroll = () => this.scrolled(side); + const onPointer = () => this.interruptScroll(); el.addEventListener("scroll", onScroll, { passive: true }); - this.cleanup.push(() => el.removeEventListener("scroll", onScroll)); + el.addEventListener("wheel", onPointer, { passive: true }); + el.addEventListener("pointerdown", onPointer); + this.cleanup.push(() => { + el.removeEventListener("scroll", onScroll); + el.removeEventListener("wheel", onPointer); + el.removeEventListener("pointerdown", onPointer); + }); } // The divider scrolls the right pane (and, when synchronized, through it the left one). const onWheel = (e: WheelEvent) => { e.preventDefault(); + this.interruptScroll(); mv.b.scrollDOM.scrollBy(e.deltaX, e.deltaY); }; this.divider.addEventListener("wheel", onWheel, { passive: false }); @@ -213,11 +223,35 @@ export class UnalignedPanes { /** Turns synchronized scrolling on (bringing the left pane in line) or off. */ setSync(on: boolean) { if (on === this.sync) return; + this.interruptScroll(); this.sync = on; this.resync(); } + scrollTo(side: Side, options: ScrollToOptions) { + this.interruptScroll(); + this.scrolling = this.sync && options.behavior === "smooth" ? side : null; + this.releaseScroll(); + this.view(side).scrollDOM.scrollTo(options); + } + + private releaseScroll() { + clearTimeout(this.scrollTimer); + if (this.scrolling) this.scrollTimer = setTimeout(() => (this.scrolling = null), 150); + } + + private interruptScroll() { + const side = this.scrolling; + this.scrolling = null; + clearTimeout(this.scrollTimer); + if (side && this.mv) { + const el = this.view(side).scrollDOM; + el.scrollTo({ top: el.scrollTop, behavior: "instant" }); + } + } + destroy() { + clearTimeout(this.scrollTimer); if (this.frame >= 0) cancelAnimationFrame(this.frame); this.resize?.disconnect(); for (const f of this.cleanup) f(); @@ -230,6 +264,13 @@ export class UnalignedPanes { } private scrolled(side: Side) { + if (this.scrolling) { + // A follower's delayed layout scroll must not cancel the leader's animation. + this.follow(this.scrolling); + this.releaseScroll(); + this.schedule(); + return; + } const el = this.view(side).scrollDOM; const moved = this.moved[side]; const echo =