diff --git a/packages/app/e2e/browser/changes-pane.spec.ts b/packages/app/e2e/browser/changes-pane.spec.ts index 655577883f..81aa9038d8 100644 --- a/packages/app/e2e/browser/changes-pane.spec.ts +++ b/packages/app/e2e/browser/changes-pane.spec.ts @@ -1019,6 +1019,16 @@ test("autofocusing an inline review keeps the Changes tab focused", async ({ pag .toBe(focusedBackground); }); +test("inline reviews keep the browser text context menu", async ({ page }) => { + const workspace = await createWorkspaceWithMountedTabDiff(); + await useUnwrappedDiffLines(page); + await openWorkspaceChanges(page, workspace); + + await startReviewOnFirstChangedLine(page); + await page.getByTestId("inline-review-editor-input").click({ button: "right" }); + await expect(page.getByTestId("diff-source-context-menu")).toHaveCount(0); +}); + test("split canvas creates a review on the changed side and keeps it in that column", async ({ page, }) => { @@ -1142,6 +1152,33 @@ test("canvas diff copies a dragged character selection without opening a review" await expect(page.getByTestId("inline-review-editor")).toHaveCount(0); }); +test("canvas diff context menu copies selections and source lines", async ({ context, page }) => { + const workspace = await createWorkspaceWithExactSelectionDiff("ABCDEFGHIJ"); + await context.grantPermissions(["clipboard-read", "clipboard-write"]); + await useUnwrappedDiffLines(page); + await openSelectionWorkspaceChanges(page, workspace); + + await dragExactAddedText(page, { startOffset: 2, endOffset: 8 }); + await rightClickFirstChangedLine(page); + await page.getByTestId("diff-source-copy-selection").click(); + await expect.poll(() => page.evaluate(() => navigator.clipboard.readText())).toBe("CDEFGH"); + + await rightClickFirstChangedLine(page); + await page.getByTestId("diff-source-copy-line").click(); + await expect.poll(() => page.evaluate(() => navigator.clipboard.readText())).toBe("ABCDEFGHIJ"); +}); + +test("canvas diff clears a selection when collapsing an earlier file", async ({ page }) => { + const workspace = await createWorkspaceWithTwoSelectionDiffs(); + await useUnwrappedDiffLines(page); + await openWorkspaceChanges(page, workspace); + + await dragExactAddedText(page, { startOffset: 2, endOffset: 8 }, 1); + await page.getByTestId("diff-file-0-toggle").click(); + await rightClickFirstChangedLine(page, 1); + await expect(page.getByTestId("diff-source-copy-selection")).toBeDisabled(); +}); + test("clicking the canvas dismisses a selection without opening a review", async ({ page }) => { const workspace = await createWorkspaceWithExactSelectionDiff("ABCDEFGHIJ"); await useUnwrappedDiffLines(page); @@ -1539,6 +1576,29 @@ async function createWorkspaceWithExactSelectionDiff(content: string): Promise { + const repo = await createTempGitRepo("changes-canvas-selection-shift-", { + files: [ + { path: "src/first.ts", content: "" }, + { path: "src/second.ts", content: "" }, + ], + }); + await Promise.all([ + writeFile(path.join(repo.path, "src/first.ts"), "FIRST\n"), + writeFile(path.join(repo.path, "src/second.ts"), "ABCDEFGHIJ\n"), + ]); + const client = await connectSeedClient(); + cleanupTasks.push({ + run: async () => { + await client.close().catch(() => undefined); + await repo.cleanup().catch(() => undefined); + }, + }); + const created = await client.createWorkspace({ source: { kind: "directory", path: repo.path } }); + if (!created.workspace) throw new Error(created.error ?? "Failed to create selection workspace"); + return { id: created.workspace.id, repoPath: repo.path }; +} + async function createWorkspaceWithManyTinyDiffs(fileCount: number): Promise { const files = Array.from({ length: fileCount }, (_, index) => ({ path: `src/file-${String(index).padStart(4, "0")}.ts`, @@ -1753,6 +1813,20 @@ async function clickFirstChangedLine(page: Page): Promise { await page.mouse.click(bodyBounds.x + 120, bodyBounds.y + lineHeight * 1.5); } +async function rightClickFirstChangedLine(page: Page, fileIndex = 0): Promise { + const body = page.getByTestId(`diff-file-${fileIndex}-body`); + const canvas = page.getByTestId("git-diff-canvas"); + const [bodyBounds, fontSize] = await Promise.all([ + body.boundingBox(), + canvas.evaluate((element) => Number.parseFloat(getComputedStyle(element).fontSize)), + ]); + if (!bodyBounds) throw new Error("Expanded diff body has no bounds"); + const lineHeight = Math.round(fontSize * 1.5); + await page.mouse.click(bodyBounds.x + 120, bodyBounds.y + lineHeight * 1.5, { + button: "right", + }); +} + async function hoverFirstChangedGutter(page: Page): Promise { const body = page.getByTestId("diff-file-0-body"); const canvas = page.getByTestId("git-diff-canvas"); @@ -1793,8 +1867,9 @@ async function deleteInlineReview(page: Page): Promise { async function dragExactAddedText( page: Page, offsets: { startOffset: number; endOffset: number }, + fileIndex = 0, ): Promise { - const body = page.getByTestId("diff-file-0-body"); + const body = page.getByTestId(`diff-file-${fileIndex}-body`); const canvas = page.getByTestId("git-diff-canvas"); const [bodyBounds, metrics] = await Promise.all([ body.boundingBox(), diff --git a/packages/app/src/git/diff-document/hit-testing.test.ts b/packages/app/src/git/diff-document/hit-testing.test.ts index edeac2b0a1..a0ed9c1578 100644 --- a/packages/app/src/git/diff-document/hit-testing.test.ts +++ b/packages/app/src/git/diff-document/hit-testing.test.ts @@ -4,6 +4,8 @@ import { characterOffsetAtPoint, hitTestDiffDocument, orderedSelection, + selectAllSource, + selectCellSource, selectedSourceText, } from "./hit-testing"; import { buildDiffDocumentModel } from "./model"; @@ -104,6 +106,64 @@ describe("diff hit testing", () => { }), ).toBeNull(); }); + + it("selects one source line for context-menu copy", () => { + const model = buildModel("copy this", { wrapLines: false }); + const cell = changedCell(model); + + expect(selectedSourceText(model, selectCellSource(model, position(model, cell, 4)))).toBe( + "copy this", + ); + }); + + it("selects all diff source in document order", () => { + const model = buildModel("first", { wrapLines: false }); + + expect(selectedSourceText(model, selectAllSource(model)!)).toBe("@@ -1,0 +1,2 @@\nfirst\ntail"); + }); + + it("selects all source from one split side without interleaving panes", () => { + const model = buildModel("unused", { files: [splitFile()], layout: "split" }); + const changedRow = model.rows.find( + (row) => row.kind === "line" && row.cells.length === 2 && row.cells.every(Boolean), + ); + expect(changedRow?.kind).toBe("line"); + + const left = changedRow!.kind === "line" ? changedRow!.cells[0]! : null; + const right = changedRow!.kind === "line" ? changedRow!.cells[1]! : null; + expect(selectedSourceText(model, selectAllSource(model, position(model, left!, 0))!)).toBe( + "@@ -1,2 +1,2 @@\nold-one\nold-two", + ); + expect(selectedSourceText(model, selectAllSource(model, position(model, right!, 0))!)).toBe( + "new-one\nnew-two", + ); + }); + + it("defaults split hunk-header select-all to the new pane", () => { + const model = buildModel("unused", { files: [splitFile()], layout: "split" }); + const header = model.rows + .filter((row) => row.kind === "line") + .flatMap((row) => row.cells) + .find((cell) => cell?.type === "header"); + expect(header).toBeDefined(); + + expect(selectedSourceText(model, selectAllSource(model, position(model, header!, 0))!)).toBe( + "new-one\nnew-two", + ); + }); + + it("falls back to the old pane for a deletion-only split hunk", () => { + const model = buildModel("unused", { files: [deletedSplitFile()], layout: "split" }); + const header = model.rows + .filter((row) => row.kind === "line") + .flatMap((row) => row.cells) + .find((cell) => cell?.type === "header"); + expect(header).toBeDefined(); + + expect(selectedSourceText(model, selectAllSource(model, position(model, header!, 0))!)).toBe( + "@@ -1,2 +1,0 @@\nold-one\nold-two", + ); + }); }); function buildModel(changedContent: string, overrides: Partial = {}) { @@ -212,6 +272,29 @@ function splitFile(): ParsedDiffFile { }; } +function deletedSplitFile(): ParsedDiffFile { + return { + path: "src/deleted.ts", + isNew: false, + isDeleted: true, + additions: 0, + deletions: 2, + hunks: [ + { + oldStart: 1, + oldCount: 2, + newStart: 1, + newCount: 0, + lines: [ + { type: "header", content: "@@ -1,2 +1,0 @@" }, + { type: "remove", content: "old-one" }, + { type: "remove", content: "old-two" }, + ], + }, + ], + }; +} + function reviewActionsForFirstAddition(): NonNullable< BuildDiffDocumentModelInput["reviewActions"] > { diff --git a/packages/app/src/git/diff-document/hit-testing.ts b/packages/app/src/git/diff-document/hit-testing.ts index ea1239e375..ed0519e281 100644 --- a/packages/app/src/git/diff-document/hit-testing.ts +++ b/packages/app/src/git/diff-document/hit-testing.ts @@ -105,6 +105,64 @@ export function selectedSourceText(model: DiffDocumentModel, selection: DiffSele .join("\n"); } +export function selectCellSource( + model: DiffDocumentModel, + position: DiffCharacterPosition, +): DiffSelection { + const row = model.rows[position.rowIndex]; + const cell = row?.kind === "line" ? row.cells[position.cellIndex] : null; + const end = cell?.content.length ?? position.sourceOffset; + return { + anchor: { ...position, sourceOffset: 0 }, + focus: { ...position, sourceOffset: end }, + }; +} + +export function selectAllSource( + model: DiffDocumentModel, + preferredPosition?: DiffCharacterPosition, +): DiffSelection | null { + let first: DiffCharacterPosition | null = null; + let last: DiffCharacterPosition | null = null; + const preferredRow = preferredPosition ? model.rows[preferredPosition.rowIndex] : null; + const preferredCell = + preferredPosition && preferredRow?.kind === "line" + ? preferredRow.cells[preferredPosition.cellIndex] + : null; + let splitCellIndex: number | null = null; + if (model.layout === "split") { + const hasNewSource = model.rows.some( + (row) => row.kind === "line" && row.cells[1] && row.cells[1].type !== "header", + ); + splitCellIndex = hasNewSource ? 1 : 0; + if (preferredPosition && preferredCell && preferredCell.type !== "header") { + splitCellIndex = preferredPosition.cellIndex; + } + } + for (const row of model.rows) { + if (row.kind !== "line") continue; + row.cells.forEach((cell, cellIndex) => { + if (!cell || (splitCellIndex !== null && cellIndex !== splitCellIndex)) return; + const side = cell.reviewTarget?.side ?? (cellIndex === 0 ? "old" : "new"); + first ??= { + fileIndex: row.fileIndex, + rowIndex: row.index, + cellIndex, + side, + sourceOffset: 0, + }; + last = { + fileIndex: row.fileIndex, + rowIndex: row.index, + cellIndex, + side, + sourceOffset: cell.content.length, + }; + }); + } + return first && last ? { anchor: first, focus: last } : null; +} + function comparePosition(left: DiffCharacterPosition, right: DiffCharacterPosition): number { return ( left.rowIndex - right.rowIndex || diff --git a/packages/app/src/git/diff-document/surface.web.tsx b/packages/app/src/git/diff-document/surface.web.tsx index 978df714e3..0b24be0610 100644 --- a/packages/app/src/git/diff-document/surface.web.tsx +++ b/packages/app/src/git/diff-document/surface.web.tsx @@ -2,7 +2,16 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } fr import { useTranslation } from "react-i18next"; import type { ViewStyle } from "react-native"; import { DomOverlayScrollbar } from "@/components/ui/overlay-scrollbar/dom-overlay-scrollbar"; +import { + ContextMenu, + ContextMenuContent, + ContextMenuItem, + ContextMenuSeparator, + ContextMenuTrigger, +} from "@/components/ui/context-menu"; +import { useToast } from "@/contexts/toast-context"; import { InlineReviewAddButton, InlineReviewThread } from "@/review"; +import { copyToClipboard } from "@/utils/copy-to-clipboard"; import type { ReviewableDiffTarget } from "@/utils/diff-layout"; import { DocumentFileHeader } from "./document-file-header"; import { @@ -11,7 +20,12 @@ import { diffMaterializationWindow, resolveVisibleFileSections, } from "./header-layout"; -import { hitTestDiffDocument, selectedSourceText } from "./hit-testing"; +import { + hitTestDiffDocument, + selectAllSource, + selectCellSource, + selectedSourceText, +} from "./hit-testing"; import { retainHorizontalOffsetMapForPaths } from "./horizontal-offsets"; import { HorizontalScroll } from "./horizontal-scroll.web"; import { buildDiffDocumentModel, FILE_HEADER_HEIGHT, resolveRelayoutScrollTop } from "./model"; @@ -33,6 +47,7 @@ const RESIZE_SETTLE_DELAY_MS = 120; export function DiffSurface(props: DiffSurfaceProps) { const { t } = useTranslation(); + const toast = useToast(); const workspaceCache = useDiffDocumentWorkspaceCache(); const rootRef = useRef(null); const canvasRef = useRef(null); @@ -75,6 +90,8 @@ export function DiffSurface(props: DiffSurfaceProps) { const [interactionFiles, setInteractionFiles] = useState< ReturnType["files"] >([]); + const [hasSelection, setHasSelection] = useState(false); + const [contextHit, setContextHit] = useState | null>(null); const [hoveredAffordance, setHoveredAffordance] = useState<{ hit: Extract; left: number; @@ -488,13 +505,13 @@ export function DiffSurface(props: DiffSurfaceProps) { }, [schedulePaint], ); - const pointHit = useCallback((event: React.PointerEvent) => { + const pointHitAt = useCallback((clientX: number, clientY: number) => { const currentModel = modelRef.current; const root = rootRef.current; if (!currentModel || !root) return null; const bounds = root.getBoundingClientRect(); - const x = event.clientX - bounds.left; - const documentY = event.clientY - bounds.top + scrollTopRef.current; + const x = clientX - bounds.left; + const documentY = clientY - bounds.top + scrollTopRef.current; const file = currentModel.files.find( (entry) => entry.top <= documentY && documentY < entry.bottom, ); @@ -505,6 +522,27 @@ export function DiffSurface(props: DiffSurfaceProps) { horizontalOffset: file ? (horizontalOffsetsRef.current.get(file.path) ?? 0) : 0, }); }, []); + const pointHit = useCallback( + (event: React.PointerEvent) => pointHitAt(event.clientX, event.clientY), + [pointHitAt], + ); + const setSelection = useCallback( + (selection: DiffSelection | null) => { + selectionRef.current = selection; + const currentModel = modelRef.current; + setHasSelection( + Boolean( + selection && currentModel && selectedSourceText(currentModel, selection).length > 0, + ), + ); + schedulePaint(); + }, + [schedulePaint], + ); + useEffect(() => { + dragRef.current = null; + setSelection(null); + }, [props.collapsedFilePaths, props.displayPreferences.layout, props.files, setSelection]); const pointerDown = useCallback( (event: React.PointerEvent) => { if (event.button !== 0) return; @@ -518,8 +556,7 @@ export function DiffSurface(props: DiffSurfaceProps) { return; const dismissSelectionOnClick = selectionRef.current !== null; if (dismissSelectionOnClick) { - selectionRef.current = null; - schedulePaint(); + setSelection(null); } const hit = pointHit(event); if (hit?.kind !== "cell") return; @@ -530,12 +567,11 @@ export function DiffSurface(props: DiffSurfaceProps) { moved: false, dismissSelectionOnClick, }; - selectionRef.current = { anchor: hit.position, focus: hit.position }; + setSelection({ anchor: hit.position, focus: hit.position }); event.currentTarget.focus(); event.currentTarget.setPointerCapture(event.pointerId); - schedulePaint(); }, - [pointHit, schedulePaint], + [pointHit, setSelection], ); const updateActiveHeader = useCallback( (target: EventTarget | null) => { @@ -589,10 +625,9 @@ export function DiffSurface(props: DiffSurfaceProps) { setHoveredAffordance(null); } if (!drag || hit?.kind !== "cell") return; - selectionRef.current = { anchor: drag.anchor, focus: hit.position }; - schedulePaint(); + setSelection({ anchor: drag.anchor, focus: hit.position }); }, - [pointHit, schedulePaint, updateActiveHeader, viewport.width], + [pointHit, setSelection, updateActiveHeader, viewport.width], ); const pointerUp = useCallback( (event: React.PointerEvent) => { @@ -612,8 +647,7 @@ export function DiffSurface(props: DiffSurfaceProps) { }) : false; if (drag && !moved && drag.dismissSelectionOnClick) { - selectionRef.current = null; - schedulePaint(); + setSelection(null); return; } if ( @@ -624,12 +658,11 @@ export function DiffSurface(props: DiffSurfaceProps) { hit.target && reviewActions ) { - selectionRef.current = null; + setSelection(null); reviewActions.onStartComment(hit.target); - schedulePaint(); } }, - [pointHit, reviewActions, schedulePaint], + [pointHit, reviewActions, setSelection], ); const cancelPointer = useCallback(() => { dragRef.current = null; @@ -643,6 +676,61 @@ export function DiffSurface(props: DiffSurfaceProps) { }, [model], ); + const prepareContextMenu = useCallback( + (event: React.MouseEvent) => { + const hit = pointHitAt(event.clientX, event.clientY); + setContextHit(hit?.kind === "cell" ? hit : null); + }, + [pointHitAt], + ); + const writeSourceText = useCallback( + (text: string) => { + void copyToClipboard(text) + .then(() => toast.copied(t("common.states.copied"))) + .catch(() => toast.error(t("common.errors.unableToCopy"))); + }, + [t, toast], + ); + const copySelectedSource = useCallback(() => { + const currentModel = modelRef.current; + const selection = selectionRef.current; + if (currentModel && selection) writeSourceText(selectedSourceText(currentModel, selection)); + }, [writeSourceText]); + const copyContextLine = useCallback(() => { + const currentModel = modelRef.current; + if (!currentModel || !contextHit) return; + const selection = selectCellSource(currentModel, contextHit.position); + writeSourceText(selectedSourceText(currentModel, selection)); + }, [contextHit, writeSourceText]); + const selectAll = useCallback(() => { + const currentModel = modelRef.current; + if (currentModel) { + setSelection( + selectAllSource(currentModel, contextHit?.position ?? selectionRef.current?.focus), + ); + } + }, [contextHit?.position, setSelection]); + const keyDown = useCallback( + (event: React.KeyboardEvent) => { + const target = event.target; + if ( + target instanceof Element && + target.closest('input, textarea, [contenteditable="true"]') + ) { + return; + } + if ((event.metaKey || event.ctrlKey) && event.key.toLowerCase() === "a") { + event.preventDefault(); + selectAll(); + return; + } + if (event.key === "Escape" && selectionRef.current) { + event.preventDefault(); + setSelection(null); + } + }, + [selectAll, setSelection], + ); const rootStyle = useMemo( () => ({ ...ROOT_STYLE, background: props.palette.surface }), [props.palette.surface], @@ -672,7 +760,7 @@ export function DiffSurface(props: DiffSurfaceProps) { [desiredTypography, loadedTypography], ); - return ( + const surface = (
@@ -752,6 +842,34 @@ export function DiffSurface(props: DiffSurfaceProps) { ) : null}
); + + return ( + + + {surface} + + + + {t("common.actions.copy")} + + + {t("common.actions.copyLine")} + + + + {t("common.actions.selectAll")} + + + + ); } function shiftCanvasPixels(input: { @@ -820,7 +938,7 @@ function WebFileHeaderSection({ [file.bottom, file.top], ); return ( -
+
+