From 855649490c25ac5d561e1ded987d72af7b07d3fa Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 8 May 2026 13:32:10 -0500 Subject: [PATCH] Add diff hunk navigation and copy --- .changeset/diff-hunk-palette.md | 5 + README.md | 3 + src/App.tsx | 76 ++++++++++++++++ src/appCommands.ts | 48 ++++++++++ src/keymap/diffView.ts | 8 ++ src/ui/FooterHints.tsx | 1 + src/ui/PullRequestDiffPane.tsx | 157 ++++++++++++++++++++++++++++++-- src/ui/diff.ts | 133 ++++++++++++++++++++++++++- test/appCommands.test.ts | 23 +++++ test/diffStacking.test.ts | 75 +++++++++++++++ 10 files changed, 519 insertions(+), 10 deletions(-) create mode 100644 .changeset/diff-hunk-palette.md diff --git a/.changeset/diff-hunk-palette.md b/.changeset/diff-hunk-palette.md new file mode 100644 index 0000000..ff03f3b --- /dev/null +++ b/.changeset/diff-hunk-palette.md @@ -0,0 +1,5 @@ +--- +"@kitlangton/ghui": minor +--- + +Add diff hunk navigation, selected-hunk copy, and selected-file diff copy commands to the command palette and diff keymap. diff --git a/README.md b/README.md index dd27453..893ced7 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,9 @@ running. - `f`: open the changed-files navigator while viewing a diff - `left` / `right`: choose the deleted or added side while in split diff comment mode - `[` / `]`: switch files while viewing or commenting on a diff +- `{` / `}`: navigate diff hunks; the active hunk shows a thin gutter line +- `y`: copy the selected diff hunk +- `Y`: copy the selected file diff - `s`: toggle draft or ready-for-review state - `m`: merge - `x`: close with confirmation diff --git a/src/App.tsx b/src/App.tsx index c4fdbad..f3e55fa 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -74,6 +74,7 @@ import { } from "./ui/commentEditor.js" import { buildStackedDiffFiles, + buildStackedDiffHunks, diffAnchorOnSide, diffCommentAnchorLabel, diffCommentLineLabel, @@ -82,12 +83,14 @@ import { getStackedDiffCommentAnchors, minimizeWhitespaceDiffFiles, nearestDiffAnchorForLocation, + nearestStackedDiffHunkIndexForFile, PullRequestDiffState, pullRequestDiffKey, safeDiffFileIndex, scrollTopForVisibleLine, splitPatchFiles, stackedDiffFileIndexAtLine, + stackedDiffHunkIndexAtLine, type DiffCommentAnchor, type DiffCommentKind, type DiffView, @@ -353,6 +356,7 @@ const diffFullViewAtom = Atom.make(false) const commentsViewActiveAtom = Atom.make(false) const commentsViewSelectionAtom = Atom.make(0) const diffFileIndexAtom = Atom.make(0) +const diffHunkIndexAtom = Atom.make(0) const diffScrollTopAtom = Atom.make(0) const diffRenderViewAtom = Atom.make("split") const diffWrapModeAtom = Atom.make("none") @@ -721,6 +725,7 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { const [commentsViewActive, setCommentsViewActive] = useAtom(commentsViewActiveAtom) const [commentsViewSelection, setCommentsViewSelection] = useAtom(commentsViewSelectionAtom) const [diffFileIndex, setDiffFileIndex] = useAtom(diffFileIndexAtom) + const [diffHunkIndex, setDiffHunkIndex] = useAtom(diffHunkIndexAtom) const [diffScrollTop, setDiffScrollTop] = useAtom(diffScrollTopAtom) const [diffRenderView, setDiffRenderView] = useAtom(diffRenderViewAtom) const [diffWrapMode, setDiffWrapMode] = useAtom(diffWrapModeAtom) @@ -998,6 +1003,12 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { () => buildStackedDiffFiles(readyDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth), [readyDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth], ) + const stackedDiffHunks = useMemo( + () => buildStackedDiffHunks(stackedDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth), + [stackedDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth], + ) + const selectedDiffHunkIndex = Math.max(0, Math.min(diffHunkIndex, stackedDiffHunks.length - 1)) + const selectedDiffHunk = stackedDiffHunks[selectedDiffHunkIndex] ?? null const diffCommentAnchors = useMemo( () => (diffFullView ? getStackedDiffCommentAnchors(stackedDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth) : []), [diffFullView, stackedDiffFiles, effectiveDiffRenderView, diffWrapMode, contentWidth], @@ -1368,6 +1379,7 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { useEffect(() => { setDiffFileIndex(0) + setDiffHunkIndex(0) setDiffScrollTop(0) setDiffCommentAnchorIndex(0) setDiffPreferredSide(null) @@ -1379,6 +1391,13 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { setDiffFileIndex((current) => safeDiffFileIndex(readyDiffFiles, current)) }, [readyDiffFiles.length]) + useEffect(() => { + setDiffHunkIndex((current) => { + if (stackedDiffHunks.length === 0) return 0 + return Math.max(0, Math.min(current, stackedDiffHunks.length - 1)) + }) + }, [stackedDiffHunks.length]) + useEffect(() => { setDiffCommentAnchorIndex((current) => { if (diffCommentAnchors.length === 0) return 0 @@ -1781,6 +1800,8 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { setDiffScrollTop((current) => (current === scrollTop ? current : scrollTop)) const nextIndex = Math.max(0, stackedDiffFileIndexAtLine(stackedDiffFiles, scrollTop)) setDiffFileIndex((current) => (current === nextIndex ? current : nextIndex)) + const nextHunkIndex = stackedDiffHunkIndexAtLine(stackedDiffHunks, scrollTop + DIFF_STICKY_HEADER_LINES) + if (nextHunkIndex >= 0) setDiffHunkIndex((current) => (current === nextHunkIndex ? current : nextHunkIndex)) } const scrollDetailPreviewBy = (y: number) => { @@ -1807,10 +1828,40 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { return () => globalThis.clearInterval(interval) }, [diffFullView, stackedDiffFiles]) + const selectDiffHunk = (index: number) => { + if (stackedDiffHunks.length === 0) return + const nextIndex = Math.max(0, Math.min(index, stackedDiffHunks.length - 1)) + const hunk = stackedDiffHunks[nextIndex] + if (!hunk) return + setDiffHunkIndex(nextIndex) + setDiffFileIndex(hunk.fileIndex) + setDiffCommentRangeStartIndex(null) + const targetSide = diffPreferredSide ?? selectedDiffCommentAnchor?.side + const nextAnchor = + diffCommentAnchors.find((anchor) => anchor.fileIndex === hunk.fileIndex && anchor.side === targetSide && anchor.renderLine >= hunk.renderLine) ?? + diffCommentAnchors.find((anchor) => anchor.fileIndex === hunk.fileIndex && anchor.renderLine >= hunk.renderLine) ?? + diffCommentAnchors.find((anchor) => anchor.fileIndex === hunk.fileIndex && anchor.side === targetSide) ?? + diffCommentAnchors.find((anchor) => anchor.fileIndex === hunk.fileIndex) + if (nextAnchor) setDiffCommentAnchorIndex(diffCommentAnchors.indexOf(nextAnchor)) + const scroll = diffScrollRef.current + if (scroll) { + const nextTop = Math.max(0, hunk.renderLine - DIFF_STICKY_HEADER_LINES) + suppressNextDiffCommentScrollRef.current = true + scroll.scrollTo({ x: 0, y: nextTop }) + syncDiffScrollState() + } + } + + const moveDiffHunk = (delta: number) => { + selectDiffHunk(selectedDiffHunkIndex + delta) + } + const selectDiffFile = (index: number) => { if (readyDiffFiles.length === 0) return const nextIndex = safeDiffFileIndex(readyDiffFiles, index) setDiffFileIndex(nextIndex) + const nextHunkIndex = nearestStackedDiffHunkIndexForFile(stackedDiffHunks, nextIndex) + if (nextHunkIndex !== null) setDiffHunkIndex(nextHunkIndex) setDiffCommentRangeStartIndex(null) const targetSide = diffPreferredSide ?? selectedDiffCommentAnchor?.side const nextAnchor = @@ -2423,6 +2474,21 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { .catch((error) => flashNotice(errorMessage(error))) } + const copySelectedDiffHunk = () => { + if (!selectedDiffHunk) return + void copyToClipboard(selectedDiffHunk.patch) + .then(() => flashNotice("Copied hunk diff")) + .catch((error) => flashNotice(errorMessage(error))) + } + + const copySelectedDiffFile = () => { + const file = selectedDiffHunk?.file ?? readyDiffFiles[safeDiffFileIndex(readyDiffFiles, diffFileIndex)] + if (!file) return + void copyToClipboard(file.patch) + .then(() => flashNotice(`Copied ${file.name} diff`)) + .catch((error) => flashNotice(errorMessage(error))) + } + const openPullRequestStateModal = () => { if (!selectedPullRequest || selectedPullRequest.state !== "open") return setPullRequestStateModal({ @@ -2941,6 +3007,8 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { diffWhitespaceMode, readyDiffFileCount: readyDiffFiles.length, diffFileIndex, + readyDiffHunkCount: stackedDiffHunks.length, + diffHunkIndex: selectedDiffHunkIndex, diffRangeActive: diffCommentRangeActive, selectedDiffCommentAnchorLabel: selectedDiffCommentLabel, selectedDiffCommentThreadCount: selectedDiffCommentThread.length, @@ -2990,6 +3058,9 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { toggleDiffWhitespaceMode, openChangedFilesModal, jumpDiffFile, + moveDiffHunk, + copySelectedDiffHunk, + copySelectedDiffFile, openSelectedDiffComment, toggleDiffCommentRange, moveDiffCommentThread, @@ -3309,6 +3380,10 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { reload: () => runCommandById("diff.reload"), nextThread: () => runCommandById("diff.next-thread"), previousThread: () => runCommandById("diff.previous-thread"), + nextHunk: () => runCommandById("diff.next-hunk"), + previousHunk: () => runCommandById("diff.previous-hunk"), + copyHunk: () => runCommandById("diff.copy-hunk"), + copyFileDiff: () => runCommandById("diff.copy-file"), moveAnchor: moveDiffCommentAnchor, moveAnchorToBoundary: moveDiffCommentToBoundary, alignAnchor: alignSelectedDiffCommentAnchor, @@ -3631,6 +3706,7 @@ export const App = ({ systemThemeGeneration = 0 }: AppProps) => { selectedCommentAnchor={selectedDiffCommentAnchor} selectedCommentLabel={selectedDiffCommentLabel} selectedCommentThread={selectedDiffCommentThread} + selectedHunk={selectedDiffHunk} onSelectCommentLine={selectDiffCommentLine} themeId={themeId} themeGeneration={systemThemeGeneration} diff --git a/src/appCommands.ts b/src/appCommands.ts index abb6557..3af30a1 100644 --- a/src/appCommands.ts +++ b/src/appCommands.ts @@ -29,6 +29,9 @@ interface AppCommandActions { readonly toggleDiffWhitespaceMode: () => void readonly openChangedFilesModal: () => void readonly jumpDiffFile: (delta: 1 | -1) => void + readonly moveDiffHunk: (delta: 1 | -1) => void + readonly copySelectedDiffHunk: () => void + readonly copySelectedDiffFile: () => void readonly openSelectedDiffComment: () => void readonly toggleDiffCommentRange: () => void readonly moveDiffCommentThread: (delta: 1 | -1) => void @@ -65,6 +68,8 @@ interface BuildAppCommandsInput { readonly diffWhitespaceMode: DiffWhitespaceMode readonly readyDiffFileCount: number readonly diffFileIndex: number + readonly readyDiffHunkCount: number + readonly diffHunkIndex: number readonly diffRangeActive: boolean readonly selectedDiffCommentAnchorLabel: string | null readonly selectedDiffCommentThreadCount: number @@ -94,6 +99,8 @@ export const buildAppCommands = ({ diffWhitespaceMode, readyDiffFileCount, diffFileIndex, + readyDiffHunkCount, + diffHunkIndex, diffRangeActive, selectedDiffCommentAnchorLabel, selectedDiffCommentThreadCount, @@ -108,6 +115,7 @@ export const buildAppCommands = ({ const selectedDiffLineReason = diffFullView && diffReady ? (selectedDiffCommentAnchorLabel ? null : "No diff line selected.") : diffOpenReadyReason const diffThreadReason = diffFullView && diffReady ? (hasDiffCommentThreads ? null : "No diff comments loaded.") : diffOpenReadyReason const changedFilesReason = diffFullView && diffReady ? (readyDiffFileCount > 0 ? null : "No changed files loaded.") : diffOpenReadyReason + const diffHunksReason = diffFullView && diffReady ? (readyDiffHunkCount > 0 ? null : "No diff hunks loaded.") : diffOpenReadyReason const selectedCommentReason = selectedPullRequest ? (commentsViewActive ? (hasSelectedComment ? null : "No comment selected.") : "Open comments first.") : noPullRequestReason const ownCommentReason = selectedCommentReason ?? (canEditSelectedComment ? null : "Only your own (synced) comments can be edited or deleted.") const loadMoreDisabledReason = isLoadingMorePullRequests ? "Already loading more pull requests." : hasMorePullRequests ? null : "No more pull requests loaded by this view." @@ -339,6 +347,46 @@ export const buildAppCommands = ({ disabledReason: changedFilesReason, run: () => actions.jumpDiffFile(-1), }), + defineCommand({ + id: "diff.next-hunk", + title: "Next diff hunk", + scope: "Diff", + subtitle: readyDiffHunkCount > 0 ? `${diffHunkIndex + 1}/${readyDiffHunkCount}` : "No diff hunks loaded", + shortcut: "}", + disabledReason: diffHunksReason, + keywords: ["patch", "section"], + run: () => actions.moveDiffHunk(1), + }), + defineCommand({ + id: "diff.previous-hunk", + title: "Previous diff hunk", + scope: "Diff", + subtitle: readyDiffHunkCount > 0 ? `${diffHunkIndex + 1}/${readyDiffHunkCount}` : "No diff hunks loaded", + shortcut: "{", + disabledReason: diffHunksReason, + keywords: ["patch", "section"], + run: () => actions.moveDiffHunk(-1), + }), + defineCommand({ + id: "diff.copy-hunk", + title: "Copy selected hunk diff", + scope: "Diff", + subtitle: readyDiffHunkCount > 0 ? `${diffHunkIndex + 1}/${readyDiffHunkCount}` : "No diff hunks loaded", + shortcut: "y", + disabledReason: diffHunksReason, + keywords: ["clipboard", "patch"], + run: actions.copySelectedDiffHunk, + }), + defineCommand({ + id: "diff.copy-file", + title: "Copy selected file diff", + scope: "Diff", + subtitle: readyDiffFileCount > 0 ? `${diffFileIndex + 1}/${readyDiffFileCount}` : "No diff files loaded", + shortcut: "shift-y", + disabledReason: changedFilesReason, + keywords: ["clipboard", "patch"], + run: actions.copySelectedDiffFile, + }), defineCommand({ id: "diff.open-comment-target", title: selectedDiffCommentThreadCount > 0 ? "Open selected diff thread" : "Comment on selected diff line", diff --git a/src/keymap/diffView.ts b/src/keymap/diffView.ts index bb6e82b..0bcfbdd 100644 --- a/src/keymap/diffView.ts +++ b/src/keymap/diffView.ts @@ -14,6 +14,10 @@ export interface DiffViewCtx { readonly reload: () => void readonly nextThread: () => void readonly previousThread: () => void + readonly nextHunk: () => void + readonly previousHunk: () => void + readonly copyHunk: () => void + readonly copyFileDiff: () => void readonly moveAnchor: (delta: number, opts?: { preserveViewportRow?: boolean }) => void readonly moveAnchorToBoundary: (boundary: "first" | "last") => void readonly alignAnchor: (align: DiffAlign) => void @@ -36,6 +40,10 @@ export const diffViewKeymap = Diff( { id: "diff.reload", title: "Reload diff", keys: ["r"], run: (s) => s.reload() }, { id: "diff.next-thread", title: "Next thread", keys: ["n"], run: (s) => s.nextThread() }, { id: "diff.previous-thread", title: "Previous thread", keys: ["p"], run: (s) => s.previousThread() }, + { id: "diff.next-hunk", title: "Next hunk", keys: ["}"], run: (s) => s.nextHunk() }, + { id: "diff.previous-hunk", title: "Previous hunk", keys: ["{"], run: (s) => s.previousHunk() }, + { id: "diff.copy-hunk", title: "Copy hunk", keys: ["y"], run: (s) => s.copyHunk() }, + { id: "diff.copy-file", title: "Copy file diff", keys: ["shift+y"], run: (s) => s.copyFileDiff() }, // Half-page anchor moves preserve viewport row (true vim semantics) { diff --git a/src/ui/FooterHints.tsx b/src/ui/FooterHints.tsx index 0dd209c..ed2230f 100644 --- a/src/ui/FooterHints.tsx +++ b/src/ui/FooterHints.tsx @@ -43,6 +43,7 @@ const diffViewHints = (ctx: HintsContext): readonly HintItem[] => [ { key: "v", label: ctx.diffRangeActive ? "clear" : "range" }, { key: "w", label: "wrap" }, { key: "[]", label: "files" }, + { key: "{}", label: "hunks" }, { key: "r", label: "reload" }, ] diff --git a/src/ui/PullRequestDiffPane.tsx b/src/ui/PullRequestDiffPane.tsx index 94e4c99..54d6bac 100644 --- a/src/ui/PullRequestDiffPane.tsx +++ b/src/ui/PullRequestDiffPane.tsx @@ -1,5 +1,5 @@ -import type { DiffRenderable, MouseEvent, ScrollBoxRenderable } from "@opentui/core" -import { useMemo, type Ref } from "react" +import type { DiffRenderable, LineSign, MouseEvent, ScrollBoxRenderable } from "@opentui/core" +import { useEffect, useMemo, useRef, type MutableRefObject, type Ref } from "react" import type { DiffCommentSide, PullRequestItem, PullRequestReviewComment } from "../domain.js" import { colors, lineNumberTextColor, type ThemeId } from "./colors.js" import { CommentBodyLine, commentCountText, commentMetaSegments, CommentSegmentsLine } from "./comments.js" @@ -16,6 +16,7 @@ import { type DiffWhitespaceMode, type DiffWrapMode, type PullRequestDiffState, + type StackedDiffHunk, type StackedDiffCommentAnchor, type StackedDiffFilePatch, } from "./diff.js" @@ -51,6 +52,110 @@ const FileStats = ({ stats }: { stats: DiffFileStats }) => { ) } +type MarkableSide = { + readonly getLineSigns: () => Map + readonly setLineSign: (line: number, sign: LineSign) => void + readonly clearLineSign: (line: number) => void +} + +type MarkableDiffInternals = { + readonly leftSide?: unknown + readonly rightSide?: unknown +} + +interface HunkMarkerSnapshot { + readonly fileIndex: number + readonly left: Map + readonly right: Map +} + +const hunkMarkerSign = "│" + +const hunkRangeText = (header: string | undefined) => header?.match(/^@@ [^@]+ @@/)?.[0] ?? header + +const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null + +const isMarkableSide = (side: unknown): side is MarkableSide => + isRecord(side) && typeof side.getLineSigns === "function" && typeof side.setLineSign === "function" && typeof side.clearLineSign === "function" + +const diffMarkableSides = (diff: DiffRenderable) => { + const candidate: unknown = diff + const internals: MarkableDiffInternals = isRecord(candidate) ? { leftSide: candidate.leftSide, rightSide: candidate.rightSide } : {} + return { + left: isMarkableSide(internals.leftSide) ? internals.leftSide : undefined, + right: isMarkableSide(internals.rightSide) ? internals.rightSide : undefined, + } +} + +const saveLineSigns = (side: MarkableSide | undefined, start: number, end: number) => { + const saved = new Map() + if (!side) return saved + + const signs = side.getLineSigns() + for (let line = start; line <= end; line++) { + const existing = signs.get(line) + saved.set(line, existing ? { ...existing } : null) + } + return saved +} + +const restoreLineSigns = (side: MarkableSide | undefined, saved: Map) => { + if (!side) return + + for (const [line, sign] of saved) { + if (sign) { + side.setLineSign(line, sign) + } else { + side.clearLineSign(line) + } + } +} + +const restoreHunkMarker = (diffRefs: ReadonlyMap, snapshot: HunkMarkerSnapshot | null) => { + if (!snapshot) return + const diff = diffRefs.get(snapshot.fileIndex) + if (!diff) return + const sides = diffMarkableSides(diff) + restoreLineSigns(sides.left, snapshot.left) + restoreLineSigns(sides.right, snapshot.right) + diff.requestRender() +} + +const clearHunkMarker = (diffRefs: ReadonlyMap, previousRef: MutableRefObject) => { + restoreHunkMarker(diffRefs, previousRef.current) + previousRef.current = null +} + +const applyMarkerToSide = (side: MarkableSide | undefined, hunk: StackedDiffHunk) => { + if (!side) return + + const signs = side.getLineSigns() + for (let line = hunk.lineRange.start; line <= hunk.lineRange.end; line++) { + const existing = signs.get(line) + side.setLineSign(line, { ...existing, before: hunkMarkerSign, beforeColor: colors.accent }) + } +} + +const applyHunkMarker = (diffRefs: ReadonlyMap, hunk: StackedDiffHunk | null, previousRef: MutableRefObject) => { + clearHunkMarker(diffRefs, previousRef) + if (!hunk || hunk.hunkCount <= 1) return + + const diff = diffRefs.get(hunk.fileIndex) + if (!diff) return + const sides = diffMarkableSides(diff) + if (!sides.left && !sides.right) return + + const snapshot: HunkMarkerSnapshot = { + fileIndex: hunk.fileIndex, + left: saveLineSigns(sides.left, hunk.lineRange.start, hunk.lineRange.end), + right: saveLineSigns(sides.right, hunk.lineRange.start, hunk.lineRange.end), + } + previousRef.current = snapshot + applyMarkerToSide(sides.left, hunk) + applyMarkerToSide(sides.right, hunk) + diff.requestRender() +} + const FileHeader = ({ file, index, @@ -97,6 +202,7 @@ export const PullRequestDiffPane = ({ selectedCommentAnchor, selectedCommentLabel, selectedCommentThread, + selectedHunk, onSelectCommentLine, themeId, themeGeneration, @@ -116,13 +222,21 @@ export const PullRequestDiffPane = ({ selectedCommentAnchor: StackedDiffCommentAnchor | null selectedCommentLabel: string | null selectedCommentThread: readonly PullRequestReviewComment[] + selectedHunk: StackedDiffHunk | null onSelectCommentLine: (renderLine: number, side: DiffCommentSide | null) => void themeId: ThemeId themeGeneration: number }) => { const readyFiles = diffState?._tag === "Ready" ? diffState.files : [] + const diffRefs = useRef(new Map()) + const hunkMarkerRef = useRef(null) const syntaxStyle = useMemo(() => createDiffSyntaxStyle(), [themeId, themeGeneration]) + useEffect(() => { + applyHunkMarker(diffRefs.current, selectedHunk, hunkMarkerRef) + return () => clearHunkMarker(diffRefs.current, hunkMarkerRef) + }, [selectedHunk?.fileIndex, selectedHunk?.hunkIndex, selectedHunk?.lineRange.start, selectedHunk?.lineRange.end, view, stackedFiles]) + if (!pullRequest) { return } @@ -184,11 +298,21 @@ export const PullRequestDiffPane = ({ const incomingStickyFile = stickyArrayIndex >= 0 ? stackedFiles[stickyArrayIndex + 1] : undefined const incomingHeaderDistance = incomingStickyFile ? incomingStickyFile.headerLine - stickyScrollTop : Number.POSITIVE_INFINITY const incomingFile = incomingHeaderDistance === 1 ? incomingStickyFile : undefined + const hunkLabelFor = (stackedFile: StackedDiffFilePatch | undefined) => { + if (!selectedHunk || selectedHunk.fileIndex !== stackedFile?.index) return "" + return ` hunk ${selectedHunk.hunkIndex + 1}/${selectedHunk.hunkCount} ${hunkRangeText(selectedHunk.header) ?? ""}` + } const stickyCommentLabelFor = (stackedFile: StackedDiffFilePatch | undefined) => { if (!selectedCommentAnchor) return " no lines" if (selectedCommentAnchor.fileIndex !== stackedFile?.index) return "" return ` ${selectedCommentLabel ?? diffCommentAnchorLabel(selectedCommentAnchor)}` } + const headerSuffixFor = (stackedFile: StackedDiffFilePatch | undefined) => { + const comment = stickyCommentLabelFor(stackedFile) + const hunk = hunkLabelFor(stackedFile) + if (comment && hunk) return `${comment}${hunk}` + return comment || hunk + } const stickyCommentColor = selectedCommentAnchor?.side === "LEFT" ? colors.status.failing : colors.status.passing const diffLineNumberFg = lineNumberTextColor(colors.diff.lineNumberBg, colors.text) const handleDiffMouseDown = function (this: ScrollBoxRenderable, event: MouseEvent) { @@ -211,11 +335,28 @@ export const PullRequestDiffPane = ({ {stackedFile.index > 0 ? : null} - + setDiffRef(stackedFile.index, diff)} + ref={(diff: DiffRenderable | null) => { + if (!diff && hunkMarkerRef.current?.fileIndex === stackedFile.index) { + clearHunkMarker(diffRefs.current, hunkMarkerRef) + } + if (diff) { + diffRefs.current.set(stackedFile.index, diff) + } else { + diffRefs.current.delete(stackedFile.index) + } + setDiffRef(stackedFile.index, diff) + }} diff={stackedFile.file.patch} view={view} syncScroll @@ -252,8 +393,8 @@ export const PullRequestDiffPane = ({ index={incomingFile.index} count={readyFiles.length} width={paneWidth} - suffix={stickyCommentLabelFor(incomingFile)} - suffixColor={stickyCommentColor} + suffix={headerSuffixFor(incomingFile)} + suffixColor={stickyCommentLabelFor(incomingFile) ? stickyCommentColor : colors.count} /> @@ -265,8 +406,8 @@ export const PullRequestDiffPane = ({ index={stickyFile.index} count={readyFiles.length} width={paneWidth} - suffix={stickyCommentLabelFor(stickyFile)} - suffixColor={stickyCommentColor} + suffix={headerSuffixFor(stickyFile)} + suffixColor={stickyCommentLabelFor(stickyFile) ? stickyCommentColor : colors.count} /> diff --git a/src/ui/diff.ts b/src/ui/diff.ts index 663ece4..c0029a7 100644 --- a/src/ui/diff.ts +++ b/src/ui/diff.ts @@ -21,6 +21,18 @@ export interface DiffFilePatch { readonly patch: string } +export interface DiffHunkPatch { + readonly header: string + readonly patch: string + readonly startLine: number +} + +export interface DiffHunkLineRange { + /** Logical line-sign coordinates inside the DiffRenderable side, not wrapped visual rows. */ + readonly start: number + readonly end: number +} + export interface DiffFileStats { readonly additions: number readonly deletions: number @@ -34,6 +46,17 @@ export interface StackedDiffFilePatch { readonly diffHeight: number } +export interface StackedDiffHunk { + readonly file: DiffFilePatch + readonly fileIndex: number + readonly hunkIndex: number + readonly hunkCount: number + readonly header: string + readonly patch: string + readonly lineRange: DiffHunkLineRange + readonly renderLine: number +} + export interface DiffCommentAnchor { readonly path: string readonly line: number @@ -323,6 +346,74 @@ export const splitPatchFiles = (patch: string): readonly DiffFilePatch[] => { }) } +export const splitPatchHunks = (patch: string): readonly DiffHunkPatch[] => { + const lines = patch.trimEnd().split("\n") + const hunkIndexes = lines.map((line, index) => ({ line, index })).filter(({ line }) => hunkHeaderPattern.test(line)) + if (hunkIndexes.length === 0) return [] + + const fileHeader = lines.slice(0, hunkIndexes[0]!.index).join("\n") + return hunkIndexes.map(({ line, index }, hunkIndex) => { + const end = hunkIndex + 1 < hunkIndexes.length ? hunkIndexes[hunkIndex + 1]!.index : lines.length + const hunkBody = lines.slice(index, end).join("\n") + return { + header: line, + patch: fileHeader.length > 0 ? `${fileHeader}\n${hunkBody}` : hunkBody, + startLine: index, + } + }) +} + +const patchLogicalLineCount = (lines: readonly string[], view: DiffView) => { + let count = 0 + let inHunk = false + let deletions = 0 + let additions = 0 + + const flushChangeBlock = () => { + if (deletions === 0 && additions === 0) return + count += view === "split" ? Math.max(deletions, additions) : deletions + additions + deletions = 0 + additions = 0 + } + + for (const line of lines) { + if (line.startsWith("@@")) { + flushChangeBlock() + inHunk = true + continue + } + + if (!inHunk) continue + + const firstChar = line[0] + if (firstChar === "\\") continue + if (firstChar === "-") { + deletions++ + continue + } + if (firstChar === "+") { + additions++ + continue + } + if (firstChar === " ") { + flushChangeBlock() + count++ + } + } + + flushChangeBlock() + return count +} + +export const patchHunkLineRange = (patch: string, hunk: DiffHunkPatch, view: DiffView): DiffHunkLineRange => { + const lines = patch.trimEnd().split("\n") + const nextHunkIndex = lines.findIndex((line, index) => index > hunk.startLine && hunkHeaderPattern.test(line)) + const endLine = nextHunkIndex >= 0 ? nextHunkIndex : lines.length + const start = patchLogicalLineCount(lines.slice(0, hunk.startLine), view) + const lineCount = patchLogicalLineCount(lines.slice(hunk.startLine, endLine), view) + return { start, end: Math.max(start, start + lineCount - 1) } +} + export const pullRequestDiffKey = (pullRequest: PullRequestItem) => `${pullRequest.repository}#${pullRequest.number}:${pullRequest.headRefOid}` export const safeDiffFileIndex = (files: readonly DiffFilePatch[], index: number) => (files.length > 0 ? Math.max(0, Math.min(index, files.length - 1)) : 0) @@ -646,7 +737,7 @@ const patchLineNumberGutterWidth = (lines: readonly string[]) => { return Math.max(3, digits + 2) + (hasSigns ? 2 : 0) } -export const patchRenderableLineCount = (patch: string, view: DiffView, wrapMode: DiffWrapMode, width: number) => { +const patchRenderableLineCountRaw = (patch: string, view: DiffView, wrapMode: DiffWrapMode, width: number) => { const lines = patch.split("\n") const contentWidth = diffContentWidth(lines, view, width) let count = 0 @@ -700,5 +791,43 @@ export const patchRenderableLineCount = (patch: string, view: DiffView, wrapMode } flushChangeBlock() - return Math.max(1, count) + return count +} + +export const patchRenderableLineCount = (patch: string, view: DiffView, wrapMode: DiffWrapMode, width: number) => { + return Math.max(1, patchRenderableLineCountRaw(patch, view, wrapMode, width)) +} + +export const patchHunkRenderableOffset = (patch: string, hunk: DiffHunkPatch, view: DiffView, wrapMode: DiffWrapMode, width: number) => { + const beforeHunk = patch.split("\n").slice(0, hunk.startLine).join("\n") + return patchRenderableLineCountRaw(beforeHunk, view, wrapMode, width) +} + +export const buildStackedDiffHunks = (stackedFiles: readonly StackedDiffFilePatch[], view: DiffView, wrapMode: DiffWrapMode, width: number): readonly StackedDiffHunk[] => { + const fileHunks = stackedFiles.map((stackedFile) => ({ stackedFile, hunks: splitPatchHunks(stackedFile.file.patch) })) + const hunkCount = fileHunks.reduce((count, { hunks }) => count + hunks.length, 0) + let hunkIndex = 0 + + return fileHunks.flatMap(({ stackedFile, hunks }) => + hunks.map((hunk) => ({ + file: stackedFile.file, + fileIndex: stackedFile.index, + hunkIndex: hunkIndex++, + hunkCount, + header: hunk.header, + patch: hunk.patch, + lineRange: patchHunkLineRange(stackedFile.file.patch, hunk, view), + renderLine: stackedFile.diffStartLine + patchHunkRenderableOffset(stackedFile.file.patch, hunk, view, wrapMode, width), + })), + ) +} + +export const stackedDiffHunkIndexAtLine = (hunks: readonly StackedDiffHunk[], line: number) => + hunks.findIndex((hunk, index) => hunk.renderLine <= line && (hunks[index + 1]?.renderLine ?? Number.POSITIVE_INFINITY) > line) + +export const nearestStackedDiffHunkIndexForFile = (hunks: readonly StackedDiffHunk[], fileIndex: number) => { + const exactIndex = hunks.findIndex((hunk) => hunk.fileIndex === fileIndex) + if (exactIndex >= 0) return exactIndex + const nextIndex = hunks.findIndex((hunk) => hunk.fileIndex > fileIndex) + return nextIndex >= 0 ? nextIndex : null } diff --git a/test/appCommands.test.ts b/test/appCommands.test.ts index cbd5afc..843609d 100644 --- a/test/appCommands.test.ts +++ b/test/appCommands.test.ts @@ -51,6 +51,8 @@ const buildCommands = (overrides: Partial[0] diffWhitespaceMode: "ignore", readyDiffFileCount: 2, diffFileIndex: 0, + readyDiffHunkCount: 3, + diffHunkIndex: 0, diffRangeActive: false, selectedDiffCommentAnchorLabel: "→ +1", selectedDiffCommentThreadCount: 0, @@ -80,6 +82,9 @@ const buildCommands = (overrides: Partial[0] toggleDiffWhitespaceMode: noop, openChangedFilesModal: noop, jumpDiffFile: noop, + moveDiffHunk: noop, + copySelectedDiffHunk: noop, + copySelectedDiffFile: noop, openSelectedDiffComment: noop, toggleDiffCommentRange: noop, moveDiffCommentThread: noop, @@ -114,6 +119,24 @@ describe("review UX commands", () => { expect(commandById("diff.changed-files", { readyDiffFileCount: 0 }).disabledReason).toBe("No changed files loaded.") }) + test("hunk navigation is available from a ready diff", () => { + const command = commandById("diff.next-hunk") + + expect(command.shortcut).toBe("}") + expect(command.disabledReason).toBeFalsy() + }) + + test("hunk navigation is disabled when no hunks are loaded", () => { + expect(commandById("diff.next-hunk", { readyDiffHunkCount: 0 }).disabledReason).toBe("No diff hunks loaded.") + }) + + test("diff copy commands are available from a ready diff", () => { + expect(commandById("diff.copy-hunk").shortcut).toBe("y") + expect(commandById("diff.copy-hunk").disabledReason).toBeFalsy() + expect(commandById("diff.copy-file").shortcut).toBe("shift-y") + expect(commandById("diff.copy-file").disabledReason).toBeFalsy() + }) + test("submit-review command is available from an open pull request", () => { const command = commandById("pull.submit-review", { diffFullView: false, diffReady: false }) diff --git a/test/diffStacking.test.ts b/test/diffStacking.test.ts index 82494ff..f7a6299 100644 --- a/test/diffStacking.test.ts +++ b/test/diffStacking.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test" import { buildStackedDiffFiles, + buildStackedDiffHunks, diffAnchorOnSide, getDiffCommentAnchors, getStackedDiffCommentAnchors, @@ -8,9 +9,12 @@ import { minimizeWhitespacePatch, nearestDiffAnchorForLocation, nearestDiffCommentAnchorIndex, + nearestStackedDiffHunkIndexForFile, patchRenderableLineCount, scrollTopForVisibleLine, splitPatchFiles, + splitPatchHunks, + stackedDiffHunkIndexAtLine, verticalDiffAnchor, } from "../src/ui/diff.ts" @@ -50,6 +54,77 @@ describe("stacked diff helpers", () => { expect(secondFileAnchor?.renderLine).toBe(stacked[1]!.diffStartLine + 1) }) + test("orders stacked hunks globally across files", () => { + const files = splitPatchFiles(`diff --git a/one.ts b/one.ts +--- a/one.ts ++++ b/one.ts +@@ -1,2 +1,2 @@ +-const one = 1 ++const one = 10 + const two = 2 +@@ -10,2 +10,2 @@ +-const ten = 10 ++const ten = 100 + const eleven = 11 +diff --git a/two.ts b/two.ts +--- a/two.ts ++++ b/two.ts +@@ -20,2 +20,2 @@ +-const twenty = 20 ++const twenty = 200 + const twentyOne = 21`) + const hunks = buildStackedDiffHunks(buildStackedDiffFiles(files, "unified", "none", 120), "unified", "none", 120) + + expect(hunks.map((hunk) => `${hunk.fileIndex}:${hunk.hunkIndex}/${hunk.hunkCount}`)).toEqual(["0:0/3", "0:1/3", "1:2/3"]) + expect(stackedDiffHunkIndexAtLine(hunks, hunks[2]!.renderLine - 1)).toBe(1) + expect(stackedDiffHunkIndexAtLine(hunks, hunks[2]!.renderLine)).toBe(2) + }) + + test("keeps hunk marker ranges in logical line-sign coordinates when wrapping", () => { + const files = splitPatchFiles(`diff --git a/wrap.ts b/wrap.ts +--- a/wrap.ts ++++ b/wrap.ts +@@ -1,1 +1,1 @@ + const first = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +@@ -10,1 +10,1 @@ + const second = true`) + const [file] = files + const hunks = splitPatchHunks(file!.patch) + const stacked = buildStackedDiffHunks(buildStackedDiffFiles(files, "unified", "word", 20), "unified", "word", 20) + + expect(hunks).toHaveLength(2) + expect(stacked[1]!.lineRange).toEqual({ start: 1, end: 1 }) + expect(stacked[1]!.renderLine).toBeGreaterThan(stacked[0]!.renderLine + 1) + }) + + test("finds the same-file or next hunk without jumping backward from hunk-less files", () => { + const files = splitPatchFiles(`diff --git a/one.ts b/one.ts +--- a/one.ts ++++ b/one.ts +@@ -1,1 +1,1 @@ +-const one = 1 ++const one = 10 +diff --git a/rename-only.ts b/rename-only.ts +similarity index 100% +rename from rename-only.ts +rename to renamed.ts +diff --git a/three.ts b/three.ts +--- a/three.ts ++++ b/three.ts +@@ -3,1 +3,1 @@ +-const three = 3 ++const three = 30 +diff --git a/mode-only.ts b/mode-only.ts +old mode 100644 +new mode 100755`) + const hunks = buildStackedDiffHunks(buildStackedDiffFiles(files, "unified", "none", 120), "unified", "none", 120) + + expect(nearestStackedDiffHunkIndexForFile(hunks, 0)).toBe(0) + expect(nearestStackedDiffHunkIndexForFile(hunks, 1)).toBe(1) + expect(nearestStackedDiffHunkIndexForFile(hunks, 2)).toBe(1) + expect(nearestStackedDiffHunkIndexForFile(hunks, 3)).toBeNull() + }) + test("keeps separate visual and color lines for wrapped unified diffs", () => { const [file] = splitPatchFiles(`diff --git a/wrap.ts b/wrap.ts --- a/wrap.ts