From b99fe1704bcddff37ca508f047f17950b68fcc98 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sat, 7 Nov 2026 12:14:09 +0800 Subject: [PATCH] fix(vcs): keep Git Log rows painted while dragging the scrollbar (#1914) Dragging the Git Log scrollbar through a long history stuttered and showed blank rows: the native scrollbar scrolls asynchronously in WebKit, so the virtual list rendered rows a frame or more after the new offset was painted. The list now uses a scrollbar drawn in the DOM (`Scrollbar` in `src/ui`) that sets the offset from pointer events and notifies scroll listeners at once, so the virtualizer renders the matching rows before the next paint. Rows are keyed by slot so a large jump updates the mounted rows instead of rebuilding them, overscan is lowered, and rows rendered mid-scroll draw their graph without edge hit targets, which come back once scrolling stops. Adds a `gitLogScrollbar` perf scenario with budgets. --- e2e/gitLogScrollbar.spec.ts | 106 ++++++++++++++++++++++++++++++++ perf/budgets.json | 3 + perf/scenarios.ts | 47 ++++++++++++++ src/features/vcs/GitLog.tsx | 119 ++++++++++++++++++++++-------------- src/ui/Scrollbar.module.css | 27 ++++++++ src/ui/Scrollbar.test.tsx | 46 ++++++++++++++ src/ui/Scrollbar.tsx | 111 +++++++++++++++++++++++++++++++++ src/ui/index.ts | 1 + 8 files changed, 414 insertions(+), 46 deletions(-) create mode 100644 e2e/gitLogScrollbar.spec.ts create mode 100644 src/ui/Scrollbar.module.css create mode 100644 src/ui/Scrollbar.test.tsx create mode 100644 src/ui/Scrollbar.tsx diff --git a/e2e/gitLogScrollbar.spec.ts b/e2e/gitLogScrollbar.spec.ts new file mode 100644 index 000000000..7ee19f28b --- /dev/null +++ b/e2e/gitLogScrollbar.spec.ts @@ -0,0 +1,106 @@ +import { expect, stubDesktop, test } from "./fixtures"; + +test("keeps a wide Git graph filled during scrollbar drags and its edges interactive afterwards", async ({ + workbench: page, +}) => { + await stubDesktop(page); + await page.evaluate(async () => { + const root = "/dev/acme-api"; + const count = 3000; + const id = (i: number) => i.toString(16).padStart(40, "0"); + const commits = Array.from({ length: count }, (_, i) => ({ + id: id(i), + shortId: id(i).slice(-8), + parents: i + 18 < count ? [id(i + 18)] : [], + refs: i === 0 ? ["HEAD", "main"] : [], + author: "Author", + email: "author@example.com", + timeMs: count - i, + summary: `Commit ${i}`, + message: `Commit ${i}`, + })); + const h = window.__ipc.handlers; + h.vcs_log = ({ query }) => { + const { skip, limit, range } = query as { + skip: number; + limit: number; + range?: string; + }; + return range ? [] : commits.slice(skip, limit ? skip + limit : undefined); + }; + h.vcs_log_refs = () => ({}); + h.vcs_branches = () => []; + h.vcs_branch_info = () => ({ + head: "main", + headCommit: id(0), + upstream: null, + ahead: 0, + behind: 0, + operation: null, + }); + h.vcs_config_get = () => null; + h.vcs_commit_files = () => []; + h.vcs_log_details_batch = ({ revisions }) => + (revisions as string[]).map((revision) => ({ + message: `Commit ${parseInt(revision, 16)}`, + containingBranches: [], + signature: "N", + signer: "", + key: "", + fingerprint: "", + })); + const storeModule = "/src/features/vcs/store.ts"; + const { useVcs } = await import(/* @vite-ignore */ storeModule); + useVcs.setState({ + available: true, + root, + repo: root, + roots: [root], + branchesLoaded: true, + }); + const contextModule = "/src/platform/context.ts"; + const { setContext } = await import(/* @vite-ignore */ contextModule); + setContext("vcsAvailable", true); + const actionsModule = "/src/platform/actions.ts"; + const { executeAction } = await import(/* @vite-ignore */ actionsModule); + await executeAction("vcs.showLog"); + }); + const list = page.getByRole("listbox", { name: "Commits" }); + await expect(list.getByRole("option").first()).toBeVisible(); + await page.evaluate(async () => { + const module = "/src/features/vcs/logStore.ts"; + const { useGitLog, loadMore } = await import(/* @vite-ignore */ module); + while (!useGitLog.getState().done) await loadMore(); + }); + + const box = (await list.boundingBox())!; + const x = box.x + box.width - 5; + await page.mouse.move(x, box.y + 8); + await page.mouse.down(); + try { + for (const fraction of [0.2, 0.7, 0.4, 0.85, 0.3]) { + await page.mouse.move(x, box.y + box.height * fraction, { steps: 8 }); + expect( + await list.evaluate((element) => { + const top = element.scrollTop; + const positions = [...element.querySelectorAll('[role="option"]')].map((row) => row.offsetTop); + return { + scrolling: top > 0, + covered: Math.min(...positions) <= top && Math.max(...positions) + 26 >= top + element.clientHeight, + }; + }), + ).toEqual({ scrolling: true, covered: true }); + } + } finally { + await page.mouse.up(); + } + + const index = await list.evaluate((element) => Math.ceil(element.scrollTop / 26)); + const row = list.locator(`[id="commit-log-${index}"]`); + await row.click(); + await expect(row).toHaveAttribute("aria-selected", "true"); + await list.press("ArrowDown"); + await expect(list.locator(`[id="commit-log-${index + 1}"]`)).toHaveAttribute("aria-selected", "true"); + // Rows drawn mid-drag get their edge hit targets back once scrolling stops. + await expect(row.getByRole("button", { name: "Collapse this fragment" }).first()).toBeAttached(); +}); diff --git a/perf/budgets.json b/perf/budgets.json index 0c2d610cd..8cd8435ab 100644 --- a/perf/budgets.json +++ b/perf/budgets.json @@ -51,6 +51,9 @@ "gitLog.tbtMs": 50, "gitLog.arrowKeyP95Ms": 20, "gitLog.arrowKeyMissedFramePct": 5, + "gitLogScrollbar.frameP95Ms": 17.5, + "gitLogScrollbar.droppedFramesPct": 2, + "gitLogScrollbar.tbtMs": 50, "resizePanels.droppedFramesPct": 2, "resizePanels.bottomDroppedFramesPct": 2, diff --git a/perf/scenarios.ts b/perf/scenarios.ts index 9c194595a..4b54b0910 100644 --- a/perf/scenarios.ts +++ b/perf/scenarios.ts @@ -316,6 +316,52 @@ const gitLog: Scenario = { }, }; +/** Scrollbar-sized jumps across a loaded graph, rather than incremental wheel scrolling. */ +const gitLogScrollbar: Scenario = { + id: "gitLogScrollbar", + title: "Drag through a loaded 6k-commit Git Log", + async run(h) { + await h.page.evaluate(async () => { + const probe = ( + window as never as { + __openideaPerf: { + fakeGitLog(n: number): void; + runAction(id: string): Promise; + }; + } + ).__openideaPerf; + probe.fakeGitLog(6000); + await probe.runAction("vcs.showLog"); + }); + const list = h.page.getByRole("listbox", { name: "Commits" }); + await list.waitFor(); + await list.evaluate(async (element) => { + for (let i = 0; i < 300 && element.scrollHeight < 6000 * 26; i++) { + element.scrollTop = element.scrollHeight; + await new Promise(requestAnimationFrame); + } + if (element.scrollHeight < 6000 * 26) throw new Error("Git Log did not finish loading"); + element.scrollTop = 0; + }); + await h.page.waitForTimeout(200); + const refresh = await h.refreshMs(); + const t0 = await h.now(); + await h.inPage((p) => p.startFrames()); + await list.evaluate(async (element) => { + const max = element.scrollHeight - element.clientHeight; + for (let i = 0; i < 180; i++) { + element.scrollTop = ((i < 90 ? i : 180 - i) / 90) * max; + await new Promise(requestAnimationFrame); + } + }); + const frames = await h.inPage((p) => p.stopFrames()); + return { + ...frameSummary(frames, refresh), + tbtMs: totalBlockingTime(await h.longTasksSince(t0)), + }; + }, +}; + /** * Dragging the splitters with an editor, the Project tree and the Git Log * open: every pointer move resizes the islands, so each frame relays them out. @@ -555,6 +601,7 @@ export const SCENARIOS: Scenario[] = [ completion, consoleOutput, gitLog, + gitLogScrollbar, resizePanels, keyRepeat, scroll, diff --git a/src/features/vcs/GitLog.tsx b/src/features/vcs/GitLog.tsx index c52e3277a..0b92336e5 100644 --- a/src/features/vcs/GitLog.tsx +++ b/src/features/vcs/GitLog.tsx @@ -6,6 +6,7 @@ import { branchesByRepo, commitKey, commitRepo, graphCommit, type RepoCommit } f import { IssueText } from "./issueLinks"; import { BranchFilterPopup } from "./BranchFilterPopup"; import { + Fragment, memo, useCallback, useContext, @@ -43,7 +44,9 @@ import { type TreeHandle, T, copyText, + hideNativeScrollbar, lazyComponent, + Scrollbar, } from "@/ui"; import { hasPrimaryModifier } from "@/platform/keymap"; import { laneColor, reachableFrom, type GraphRow, type GraphSegment } from "./graph"; @@ -1398,7 +1401,7 @@ function CommitList() { count: commits.length + (finished ? 0 : 1), getScrollElement: () => scrollRef.current, estimateSize: () => ROW, - overscan: 20, + overscan: 8, scrollMargin: ROW, }); const items = virtualizer.getVirtualItems(); @@ -1410,7 +1413,10 @@ function CommitList() { if (!finished && !loading && last && last.index >= commits.length - PAGE) void loadMore(scope ?? undefined); }, [last, commits.length, finished, loading, active, scope]); - const selectedIndex = selected ? commits.findIndex((c) => commitKey(c) === selected) : -1; + const selectedIndex = useMemo( + () => (selected ? commits.findIndex((c) => commitKey(c) === selected) : -1), + [commits, selected], + ); useLayoutEffect(() => { if (selectedIndex >= 0) virtualizer.scrollToIndex(selectedIndex, { align: "center" }); // Only on explicit reveal requests. @@ -1669,9 +1675,10 @@ function CommitList() { {error} )} +
); })} @@ -1876,7 +1886,7 @@ function rowRemoteNames(own: readonly string[] | undefined, shared: ReadonlySet< return set; } -const CommitRow = memo(function CommitRow({ +function CommitRowView({ id, commit: c, graph, @@ -1897,6 +1907,7 @@ const CommitRow = memo(function CommitRow({ targetTitle, columns, bookmarked, + interactive, }: { id: string; commit: Commit; @@ -1918,6 +1929,8 @@ const CommitRow = memo(function CommitRow({ targetTitle: (index: number, key?: string) => string; columns: LogColumn[]; bookmarked: boolean; + /** False for rows rendered mid-scroll: their graph is drawn without hit targets. */ + interactive: boolean; }) { const merge = opts.highlightMerges && c.parents.length > 1; const subjectRef = useRef(null); @@ -1981,6 +1994,7 @@ const CommitRow = memo(function CommitRow({ targetTitle={targetTitle} onFragment={(target, expand, source) => onFragment(source ?? commitKey(c), target, expand)} onNode={() => onFragment(commitKey(c), commitKey(c), false)} + interactive={interactive} /> )} {bookmarked && } @@ -2029,6 +2043,16 @@ const CommitRow = memo(function CommitRow({ ))}
); +} + +// A row that is already interactive stays so when scrolling starts; only rows +// whose content changes mid-scroll are drawn without hit targets. +const CommitRow = memo(CommitRowView, (prev, next) => { + for (const key in next) { + const k = key as keyof typeof next; + if (prev[k] !== next[k] && !(k === "interactive" && prev.interactive)) return false; + } + return true; }); function x(lane: number) { @@ -2051,6 +2075,7 @@ export function GraphCell({ onFragment, targetTitle, onNode, + interactive = true, }: { row: GraphRow; head?: boolean; @@ -2062,9 +2087,27 @@ export function GraphCell({ onFragment?: (target: string, expand: boolean, source?: string) => void; targetTitle?: (index: number, key?: string) => string; onNode?: () => void; + interactive?: boolean; }) { const cy = ROW / 2; const draw = (s: GraphSegment, i: number, up: boolean) => { + const fromX = x(s.fromX ?? s.from); + const toX = x(s.toX ?? s.to); + const path = + seg(fromX, up ? 0 : cy, toX, up ? cy : ROW) + + (s.arrow ? arrowHead(up ? fromX : toX, up ? 0 : ROW, up ? -1 : 1) : ""); + const line = ( + + ); + if (!interactive) return line; const target = (up ? row.upTargets : row.downTargets)?.[i]; const canFragment = !!target && @@ -2086,50 +2129,34 @@ export function GraphCell({ if (canNavigate) onNavigate?.(s.targetRow!, target); else if (canFragment) onFragment?.(target!, !!s.dashed, s.source); }; - const fromX = x(s.fromX ?? s.from); - const toX = x(s.toX ?? s.to); - const path = - seg(fromX, up ? 0 : cy, toX, up ? cy : ROW) + - (s.arrow ? arrowHead(up ? fromX : toX, up ? 0 : ROW, up ? -1 : 1) : ""); return ( - onHover?.(s.edge ?? `${s.source ?? ""}\0${target ?? ""}`)} - onMouseDown={(e) => e.stopPropagation()} - onClick={(e) => { - e.stopPropagation(); - activate(); - }} - onKeyDown={(e) => { - if (actionable && (e.key === "Enter" || e.key === " ")) { - e.preventDefault(); - e.stopPropagation(); - activate(); - } - }} - > - {label} - - + - + stroke="transparent" + strokeWidth={10} + style={{ pointerEvents: "stroke" }} + role={actionable ? "button" : undefined} + tabIndex={actionable ? 0 : undefined} + aria-label={actionable ? label : undefined} + onMouseEnter={() => onHover?.(s.edge ?? `${s.source ?? ""}\0${target ?? ""}`)} + onMouseDown={(e) => e.stopPropagation()} + onClick={(e) => { + e.stopPropagation(); + activate(); + }} + onKeyDown={(e) => { + if (actionable && (e.key === "Enter" || e.key === " ")) { + e.preventDefault(); + e.stopPropagation(); + activate(); + } + }} + > + {label} + + {line} + ); }; return ( diff --git a/src/ui/Scrollbar.module.css b/src/ui/Scrollbar.module.css new file mode 100644 index 000000000..7fb7a97be --- /dev/null +++ b/src/ui/Scrollbar.module.css @@ -0,0 +1,27 @@ +/* Same look as the global ::-webkit-scrollbar (global.css). */ +.track { + position: absolute; + top: 0; + right: 0; + width: 10px; + z-index: 1; +} +.thumb { + position: absolute; + left: 0; + right: 0; + top: 0; + border: 2px solid transparent; + border-radius: 6px; + background-clip: content-box; + background-color: var(--scrollbar-thumb); + will-change: transform; +} +.thumb:hover, +.thumb[data-dragging] { + background-color: var(--scrollbar-thumb-hover); +} +/* For the scroller whose vertical scrollbar `Scrollbar` draws. */ +.hideNative::-webkit-scrollbar:vertical { + width: 0; +} diff --git a/src/ui/Scrollbar.test.tsx b/src/ui/Scrollbar.test.tsx new file mode 100644 index 000000000..40a078730 --- /dev/null +++ b/src/ui/Scrollbar.test.tsx @@ -0,0 +1,46 @@ +import { act, render } from "@testing-library/react"; +import { useRef } from "react"; +import { describe, expect, it, vi } from "vitest"; +import { Scrollbar } from "./Scrollbar"; + +function Harness({ onScroll }: { onScroll: (top: number) => void }) { + const ref = useRef(null); + return ( +
+ +
onScroll(e.currentTarget.scrollTop)} /> +
+ ); +} + +describe("Scrollbar", () => { + it("notifies scroll listeners synchronously while the thumb is dragged", () => { + const onScroll = vi.fn(); + const { getByTestId, container } = render(); + const scroller = getByTestId("scroller"); + let top = 0; + Object.defineProperties(scroller, { + clientHeight: { value: 100 }, + scrollHeight: { value: 1000 }, + scrollTop: { get: () => top, set: (v: number) => (top = v) }, + }); + scroller.scrollTo = ((options: ScrollToOptions) => { + top = Math.max(0, Math.min(900, options.top ?? top)); + }) as typeof scroller.scrollTo; + act(() => { + scroller.dispatchEvent(new Event("scroll")); + }); + onScroll.mockClear(); + + const track = container.querySelector("[aria-hidden]")!; + const thumb = track.firstElementChild as HTMLElement; + track.setPointerCapture = () => {}; + thumb.dispatchEvent(new PointerEvent("pointerdown", { bubbles: true, button: 0, clientY: 10 })); + track.dispatchEvent(new PointerEvent("pointermove", { bubbles: true, clientY: 46 })); + + // A 32px thumb travels 68px of the track for 900px of content: 36px ≈ 476px. + expect(top).toBeCloseTo((36 / 68) * 900); + expect(onScroll).toHaveBeenCalledTimes(1); + expect(onScroll).toHaveBeenLastCalledWith(top); + }); +}); diff --git a/src/ui/Scrollbar.tsx b/src/ui/Scrollbar.tsx new file mode 100644 index 000000000..dd705a62f --- /dev/null +++ b/src/ui/Scrollbar.tsx @@ -0,0 +1,111 @@ +import { useEffect, useLayoutEffect, useRef, type RefObject } from "react"; +import styles from "./Scrollbar.module.css"; + +const MIN_THUMB = 32; + +/** Class for the scroller: hides its native vertical scrollbar, which `Scrollbar` replaces. */ +export const hideNativeScrollbar = styles.hideNative; + +/** + * A vertical scrollbar drawn in the DOM over `target` (place both in a + * `position: relative` box). Dragging it sets `scrollTop` from pointer events, + * so content rendered on scroll (virtual lists) lands in the same frame. The + * native one scrolls asynchronously in WebKit and shows blank rows until the + * page catches up. + * + * `contentHeight` re-measures when the scroller's content grows. + */ +export function Scrollbar({ + target, + contentHeight, +}: { + target: RefObject; + contentHeight?: number; +}) { + const track = useRef(null); + const thumb = useRef(null); + const drag = useRef<{ y: number; top: number } | null>(null); + const geometry = useRef(null); + + useLayoutEffect(() => { + geometry.current = layout(target.current, track.current, thumb.current); + }, [target, contentHeight]); + useEffect(() => { + const el = target.current; + if (!el) return; + const sync = () => { + geometry.current = layout(el, track.current, thumb.current); + }; + el.addEventListener("scroll", sync, { passive: true }); + const observer = new ResizeObserver(sync); + observer.observe(el); + return () => { + el.removeEventListener("scroll", sync); + observer.disconnect(); + }; + }, [target]); + + return ( +
{ + const el = target.current; + if (!el || e.button !== 0) return; + e.preventDefault(); + // A click on the track pages towards it, as the native scrollbar does. + if (e.target !== thumb.current) { + const { top, bottom } = thumb.current!.getBoundingClientRect(); + if (e.clientY < top) scrollNow(el, el.scrollTop - el.clientHeight); + else if (e.clientY > bottom) scrollNow(el, el.scrollTop + el.clientHeight); + return; + } + drag.current = { y: e.clientY, top: el.scrollTop }; + thumb.current!.dataset.dragging = ""; + e.currentTarget.setPointerCapture(e.pointerId); + }} + onPointerMove={(e) => { + const el = target.current; + if (!el || !drag.current) return; + const g = geometry.current; + if (g) scrollNow(el, drag.current.top + ((e.clientY - drag.current.y) / g.range) * g.max); + }} + onLostPointerCapture={() => { + drag.current = null; + delete thumb.current?.dataset.dragging; + }} + > +
+
+ ); +} + +type Geometry = { range: number; max: number }; + +/** Positions the track over `el` and the thumb at its scroll offset; null when nothing scrolls. */ +function layout(el: HTMLElement | null, t: HTMLElement | null, th: HTMLElement | null): Geometry | null { + if (!el || !t || !th) return null; + const height = el.clientHeight; + const max = el.scrollHeight - height; + t.style.top = `${el.offsetTop}px`; + t.style.height = `${height}px`; + t.style.display = max > 0 ? "" : "none"; + if (max <= 0) return null; + const size = Math.max(MIN_THUMB, (height * height) / el.scrollHeight); + const range = height - size; + th.style.height = `${size}px`; + th.style.transform = `translateY(${(Math.min(max, el.scrollTop) / max) * range}px)`; + return range > 0 ? { range, max } : null; +} + +/** + * Scrolls `el` and tells its scroll listeners at once. WebKit fires `scroll` for + * a script scroll only at a later rendering step and may paint the new offset + * first, so a virtual list would show a frame of blank rows mid-drag. + */ +function scrollNow(el: HTMLElement, top: number) { + const before = el.scrollTop; + el.scrollTo({ top, behavior: "instant" }); + if (el.scrollTop !== before) el.dispatchEvent(new Event("scroll")); +} diff --git a/src/ui/index.ts b/src/ui/index.ts index b20bcab1d..b8773d1ac 100644 --- a/src/ui/index.ts +++ b/src/ui/index.ts @@ -25,6 +25,7 @@ export * from "./Popup"; export * from "./Menu"; export * from "./Dialog"; export * from "./Splitter"; +export { Scrollbar, hideNativeScrollbar } from "./Scrollbar"; export * from "./SourcePreview"; export * from "./Feedback"; export * from "./HighlightedText";