diff --git a/apps/desktop/src/settings/DesktopClientSettings.test.ts b/apps/desktop/src/settings/DesktopClientSettings.test.ts index 90c8740f83725..111940699775d 100644 --- a/apps/desktop/src/settings/DesktopClientSettings.test.ts +++ b/apps/desktop/src/settings/DesktopClientSettings.test.ts @@ -34,6 +34,7 @@ const clientSettings: ClientSettings = { confirmThreadArchive: true, confirmThreadDelete: false, confirmThreadUnpin: false, + diffAutoAdvanceOnViewed: true, diffFilesCollapsed: true, diffIgnoreWhitespace: true, diffLayout: "stacked", diff --git a/apps/web/src/components/DiffPanel.browser.test.tsx b/apps/web/src/components/DiffPanel.browser.test.tsx index 503f9b60c1b5d..58438cc97d4cf 100644 --- a/apps/web/src/components/DiffPanel.browser.test.tsx +++ b/apps/web/src/components/DiffPanel.browser.test.tsx @@ -40,9 +40,13 @@ const state = vi.hoisted(() => ({ checkpointDiff: null as { diff: string } | null, })); -vi.mock("~/state/server", () => ({ - serverEnvironment: { configValueAtom: () => state.config }, -})); +vi.mock("~/state/server", async () => { + const { Atom } = await import("effect/reactivity"); + return { + serverEnvironment: { configValueAtom: () => state.config }, + primaryServerKeybindingsAtom: Atom.make([]), + }; +}); vi.mock("~/state/review", () => ({ reviewEnvironment: { diffPreview: ({ input }: { input: { baseRef?: string } }) => diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 9dad875a2a8a6..611faa4e8cc28 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -6,6 +6,7 @@ import { squashAtomCommandFailure, } from "@t3tools/client-runtime/state/runtime"; import { safeErrorLogAttributes } from "@t3tools/client-runtime/errors"; +import { scopedThreadKey } from "@t3tools/client-runtime/environment"; import { CHECKPOINT_DIFF_TRUNCATED_MARKER, type ReviewBranchCommitsResult, @@ -27,7 +28,7 @@ import { ChevronsUpDownIcon, } from "lucide-react"; import * as Schema from "effect/Schema"; -import { type MouseEvent, useEffect, useRef, useState } from "react"; +import { type ComponentProps, type MouseEvent, useEffect, useRef, useState } from "react"; import { useCodeViewFileReveal } from "./diffs/useCodeViewFileReveal"; import { useCodeViewScrollRestore } from "./diffs/useCodeViewScrollRestore"; import { useFilesystemReadAccess } from "~/state/filesystem"; @@ -46,6 +47,7 @@ import { useDiffPanelSessionStore, useDiffPanelStore, } from "../diffPanelStore"; +import { diffViewedEntry, diffViewedFingerprint, useDiffViewedStore } from "../diffViewedStore"; import { useLocalStorage } from "../hooks/useLocalStorage"; import { isTestChangedFile } from "@t3tools/shared/changedFileKind"; import { useHideTestFiles } from "./diffs/useHideTestFiles"; @@ -73,7 +75,24 @@ import { formatRelativeTimeLabel, formatShortTimestamp } from "../timestampForma import { DiffFilePathCopyButton } from "./DiffFilePathCopyButton"; import { DiffPanelLoadingState, DiffPanelShell, type DiffPanelMode } from "./DiffPanelShell"; import { DiffStatLabel } from "./chat/DiffStatLabel"; -import { AnnotatableCodeView, type AnnotatableCodeViewHandle } from "./diffs/AnnotatableCodeView"; +import { + AnnotatableCodeView, + type AnnotatableCodeViewHandle, + type AnnotatableCodeViewReview, +} from "./diffs/AnnotatableCodeView"; +import { + DiffCursorOutline, + DiffSingleFileToggle, + DiffViewedCount, + DiffViewedToggle, +} from "./diffs/DiffReviewControls"; +import { + DiffReviewCursorContext, + type DiffReviewFile, + useDiffReviewCursorFilePath, + useDiffReviewKeyboard, + useDiffReviewSelectedLines, +} from "./diffs/useDiffReviewKeyboard"; import { DiffFileTree } from "./diffs/DiffFileTree"; import { FileExplorerPane } from "./files/FileExplorerPane"; import { diffFileTreeEntries } from "./diffs/diffFileTree.logic"; @@ -122,7 +141,7 @@ const AUTOMATIC_BASE_REF = "__automatic_base_ref__"; const DIFF_FILE_TREE_STORAGE_KEY = "t3code.diffFileTreeOpen"; const fileEntryCache = new WeakMap< FileDiffMetadata, - { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number } + { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number; viewedFingerprint: string } >(); function getCachedFileEntry(fileDiff: FileDiffMetadata) { @@ -132,6 +151,7 @@ function getCachedFileEntry(fileDiff: FileDiffMetadata) { fileDiff, fileKey: buildFileDiffIdentityKey(fileDiff), fileVersion: buildFileDiffContentVersion(fileDiff), + viewedFingerprint: diffViewedFingerprint(fileDiff), }; fileEntryCache.set(fileDiff, entry); return entry; @@ -139,6 +159,20 @@ function getCachedFileEntry(fileDiff: FileDiffMetadata) { const EMPTY_COLLAPSED_DIFF_FILE_KEYS: ReadonlySet = new Set(); +/** The code view with the review cursor's lines selected; a cursor move redraws only this. */ +function ReviewAnnotatableCodeView( + props: Omit, "selectedLines">, +) { + const selectedLines = useDiffReviewSelectedLines(); + return ; +} + +/** Highlights the review cursor's file, falling back to the file another view selected. */ +function ReviewFileTree(props: ComponentProps) { + const cursorPath = useDiffReviewCursorFilePath(); + return ; +} + function DiffFileCollapseToggle({ filePath, fileKey, @@ -153,40 +187,43 @@ function DiffFileCollapseToggle({ onToggle: (fileKey: string) => void; }) { return ( - - { - if (!node || !load) return; - return observeIntersection( - node, - (entry) => { - if (entry.isIntersecting) load(); - }, - { rootMargin: "600px" }, - ); - }} - onClick={(event) => { - event.stopPropagation(); - onToggle(fileKey); - }} - /> - } - > - {collapsed ? ( - - ) : ( - - )} - - {collapsed ? "Expand diff" : "Collapse diff"} - + <> + + + { + if (!node || !load) return; + return observeIntersection( + node, + (entry) => { + if (entry.isIntersecting) load(); + }, + { rootMargin: "600px" }, + ); + }} + onClick={(event) => { + event.stopPropagation(); + onToggle(fileKey); + }} + /> + } + > + {collapsed ? ( + + ) : ( + + )} + + {collapsed ? "Expand diff" : "Collapse diff"} + + ); } @@ -230,6 +267,7 @@ export default function DiffPanel({ }: DiffPanelProps) { const resolvedTheme = useRenderedAppearance(); const settings = useClientSettings((settings) => ({ + diffAutoAdvanceOnViewed: settings.diffAutoAdvanceOnViewed, diffFilesCollapsed: settings.diffFilesCollapsed, diffIgnoreWhitespace: settings.diffIgnoreWhitespace, diffLayout: settings.diffLayout, @@ -686,24 +724,54 @@ export default function DiffPanel({ .getState() .updateCollapsedFileKeys(collapseScopeKey, defaultCollapsedDiffFileKeys, update); }; - const renderLoadingBoundary = () => - remainingFileCount > 0 ? ( - - ) : null; - const codeViewFiles = shownFileEntries.map(({ fileDiff, fileKey, fileVersion }) => ({ - fileDiff, - filePath: resolveFileDiffPath(fileDiff), + const isFilePending = (fileDiff: FileDiffMetadata) => + fileDiff.cacheKey?.endsWith(":pending") === true; + const viewedThreadKey = routeThreadRef ? scopedThreadKey(routeThreadRef) : null; + const viewedEntries = useDiffViewedStore((state) => + viewedThreadKey ? state.viewedByThreadKey[viewedThreadKey] : undefined, + ); + const viewedEntrySet = new Set(viewedEntries); + const isFileViewed = (fileDiff: FileDiffMetadata) => + viewedEntrySet.has( + diffViewedEntry( + resolveFileDiffPath(fileDiff), + getCachedFileEntry(fileDiff).viewedFingerprint, + ), + ); + const setFileViewed = (fileDiff: FileDiffMetadata, viewed: boolean) => { + if (!viewedThreadKey) return; + useDiffViewedStore + .getState() + .setFileViewed( + viewedThreadKey, + resolveFileDiffPath(fileDiff), + getCachedFileEntry(fileDiff).viewedFingerprint, + viewed, + ); + }; + const pendingFileKeys = new Set( + renderableFileEntries + .filter(({ fileDiff }) => isFilePending(fileDiff)) + .map((file) => file.fileKey), + ); + const setDiffFileCollapsed = (fileKey: string, collapsed: boolean) => { + const file = renderableFileEntries.find((candidate) => candidate.fileKey === fileKey); + if (!collapsed && lazySource && file) requestFile(resolveFileDiffPath(file.fileDiff)); + updateCollapsedDiffFiles((current) => { + if (current.has(fileKey) === collapsed) return current; + const next = new Set(current); + if (collapsed) next.add(fileKey); + else next.delete(fileKey); + return next; + }); + }; + const reviewFiles: DiffReviewFile[] = shownFileEntries.map(({ fileDiff, fileKey }) => ({ fileKey, - fileVersion, - collapsed: - collapsedDiffFileKeys.has(fileKey) || fileDiff.cacheKey?.endsWith(":pending") === true, + filePath: resolveFileDiffPath(fileDiff), + fileDiff, + rendered: !isFilePending(fileDiff), })); - const pendingFiles = renderableFiles.filter( - (file) => file.cacheKey?.endsWith(":pending") && isFileShown(file), - ); - const allDiffFilesCollapsed = areAllDiffFilesCollapsed(shownFileKeys, collapsedDiffFileKeys); - const diffLineStat = sourceLineStat ?? getDiffLineStat(renderableFiles); - const fileTreeEntries = diffFileTreeEntries(renderableFiles.filter(isFileShown)); + const viewedFileCount = shownFileEntries.filter(({ fileDiff }) => isFileViewed(fileDiff)).length; const codeViewReveal = useCodeViewFileReveal(); // Lazy files that sat above the saved position load again only when asked for. const codeViewScroll = useCodeViewScrollRestore( @@ -711,28 +779,71 @@ export default function DiffPanel({ !isRefreshingDiff && remainingFileCount === 0, remainingFileCount > 0 ? loadNextFiles : null, ); - const attachCodeView = useStableCallback((handle: AnnotatableCodeViewHandle | null) => { + const viewerHandleRef = useRef(null); + const attachViewer = useStableCallback((handle: AnnotatableCodeViewHandle | null) => { + viewerHandleRef.current = handle; codeViewScroll.attach(handle); codeViewReveal.attach(handle); }); - const flushCodeView = useStableCallback(() => { + const revealFileAt = (index: number, align?: "start" | "nearest") => { + const file = renderableFileEntries[index]; + if (!file) return; + codeViewScroll.discard(); + if (lazySource) requestFile(resolveFileDiffPath(file.fileDiff)); + codeViewReveal.request(file.fileKey, undefined, align); + }; + const reviewRef = useRef(null); + const { rootRef: reviewRootRef, ...review } = useDiffReviewKeyboard({ + scopeKey: collapseScopeKey, + files: reviewFiles, + diffStyle: diffLayout === "split" ? "split" : "unified", + canLoadFullFiles: currentLoadDiffFiles !== undefined, + autoAdvanceOnViewed: settings.diffAutoAdvanceOnViewed, + getViewer: () => viewerHandleRef.current, + isCollapsed: (fileKey) => collapsedDiffFileKeys.has(fileKey) || pendingFileKeys.has(fileKey), + setCollapsed: setDiffFileCollapsed, + isViewed: (file) => isFileViewed(file.fileDiff), + setViewed: (file, viewed) => setFileViewed(file.fileDiff, viewed), + revealFile: (file, align) => + revealFileAt( + renderableFileEntries.findIndex(({ fileKey }) => fileKey === file.fileKey), + align, + ), + beginComment: (fileKey, range) => reviewRef.current?.beginComment(fileKey, range), + }); + const onPostRender = useStableCallback(() => { codeViewScroll.flush(); codeViewReveal.flush(); + review.flushScroll(); }); + const renderLoadingBoundary = () => + review.singleFileKey === undefined && remainingFileCount > 0 ? ( + + ) : null; + const codeViewFiles = shownFileEntries + .filter(({ fileKey }) => review.singleFileKey === undefined || fileKey === review.singleFileKey) + .map(({ fileDiff, fileKey, fileVersion }) => ({ + fileDiff, + filePath: resolveFileDiffPath(fileDiff), + fileKey, + fileVersion, + collapsed: + isFilePending(fileDiff) || + (fileKey !== review.singleFileKey && collapsedDiffFileKeys.has(fileKey)), + })); + const pendingFiles = renderableFiles.filter((file) => isFilePending(file) && isFileShown(file)); + const allDiffFilesCollapsed = areAllDiffFilesCollapsed(shownFileKeys, collapsedDiffFileKeys); + const diffLineStat = sourceLineStat ?? getDiffLineStat(renderableFiles); + const fileTreeEntries = diffFileTreeEntries(renderableFiles.filter(isFileShown)); const revealDiffFile = (filePath: string) => { const index = renderableFileEntries.findIndex( (candidate) => resolveFileDiffPath(candidate.fileDiff) === filePath, ); const file = renderableFileEntries[index]; if (!file) return false; - codeViewScroll.discard(); - updateCollapsedDiffFiles((current) => { - const next = new Set(current); - next.delete(file.fileKey); - return next; - }); - if (lazySource) requestFile(filePath); - codeViewReveal.request(file.fileKey); + setDiffFileCollapsed(file.fileKey, false); + review.selectFile(filePath); + revealFileAt(index); return true; }; @@ -777,20 +888,6 @@ export default function DiffPanel({ }, }); }; - const toggleDiffFileCollapsed = useStableCallback((fileKey: string) => { - const file = renderableFileEntries.find((file) => file.fileKey === fileKey); - if (lazySource && file) requestFile(resolveFileDiffPath(file.fileDiff)); - const pending = file?.fileDiff.cacheKey?.endsWith(":pending") === true; - updateCollapsedDiffFiles((current) => { - const next = new Set(current); - if (pending || next.has(fileKey)) { - next.delete(fileKey); - } else { - next.add(fileKey); - } - return next; - }); - }); // Find can ask again before the unfolded file reaches the viewer, so this must never fold. const unfoldDiffFile = useStableCallback((fileKey: string) => { updateCollapsedDiffFiles((current) => { @@ -803,6 +900,7 @@ export default function DiffPanel({ const toggleDiffFileCollapse = () => { codeViewScroll.discard(); + review.showAllFiles(); setCodeViewRevision((current) => current + 1); updateCollapsedDiffFiles((current) => toggleAllDiffFiles(shownFileKeys, current)); }; @@ -1149,6 +1247,11 @@ export default function DiffPanel({ headerCommitSha !== null && "gap-0", )} > + {shownFileEntries.length > 0 ? ( + + viewed + + ) : null} {codeViewFiles.length > 0 || (isGitSelection && selectedGitSource?.files?.length) ? ( + {diffFileKeys.length > 0 && ( + + )} - {!activeThread ? ( -
- Select a thread to inspect turn diffs. -
- ) : !isGitRepo && !toolCallSelection ? ( -
- Turn diffs are unavailable because this project is not a git repository. -
- ) : selectedRunId !== null && orderedTurnDiffSummaries.length === 0 ? ( -
- No completed turns yet. -
- ) : isGitSelection && !canReadFiles ? ( - fileAccess.isPending ? ( - - ) : ( + + + {!activeThread ? (
- {fileAccess.error ?? "This connection cannot read local diffs."} + Select a thread to inspect turn diffs.
- ) - ) : ( - <> -
- {preview?.incomplete && ( -

- {selectedPatchError - ? "Showing tracked changes. Untracked changes could not be loaded. Refresh to retry." - : "Showing tracked changes. Checking untracked files..."} -

- )} - {isSelectedPatchTruncated && !lazySource && ( -

- This preview exceeds the size limit. Changes shown are incomplete. - {isGitSelection && selectedGitSource?.files && !preview?.incomplete - ? " Totals include all changes." - : ""} -

- )} - {omittedToolCallFiles > 0 && ( -

- This tool call changed {recordedToolCallFileCount} files. The recorded patch omits{" "} - {omittedToolCallFiles} of them. -

- )} - {selectedPatchError && !renderablePatch && ( -
-

{selectedPatchError}

-
- )} - {!renderablePatch && toolCallPreview && toolCallPreview.content.length > 0 ? ( -
-

- The provider recorded file content without a diff. + ) : !isGitRepo && !toolCallSelection ? ( +

+ Turn diffs are unavailable because this project is not a git repository. +
+ ) : selectedRunId !== null && orderedTurnDiffSummaries.length === 0 ? ( +
+ No completed turns yet. +
+ ) : isGitSelection && !canReadFiles ? ( + fileAccess.isPending ? ( + + ) : ( +
+ {fileAccess.error ?? "This connection cannot read local diffs."} +
+ ) + ) : ( + <> +
+ {preview?.incomplete && ( +

+ {selectedPatchError + ? "Showing tracked changes. Untracked changes could not be loaded. Refresh to retry." + : "Showing tracked changes. Checking untracked files..."}

- {toolCallPreview.content.map(({ label, text }) => ( -
-

{label}

- 
-                      {text === "" ? "Empty content." : text}
-
-
- ))} -
- ) : !renderablePatch && !lazySource ? ( - isLoadingSelectedPatch ? ( - - ) : ( -
-

- {hasNoNetChanges - ? "No net changes in this selection." - : "No patch available for this selection."} + )} + {isSelectedPatchTruncated && !lazySource && ( +

+ This preview exceeds the size limit. Changes shown are incomplete. + {isGitSelection && selectedGitSource?.files && !preview?.incomplete + ? " Totals include all changes." + : ""} +

+ )} + {omittedToolCallFiles > 0 && ( +

+ This tool call changed {recordedToolCallFileCount} files. The recorded patch omits{" "} + {omittedToolCallFiles} of them. +

+ )} + {selectedPatchError && !renderablePatch && ( +
+

{selectedPatchError}

+
+ )} + {!renderablePatch && toolCallPreview && toolCallPreview.content.length > 0 ? ( +
+

+ The provider recorded file content without a diff.

+ {toolCallPreview.content.map(({ label, text }) => ( +
+

{label}

+ 
+                        {text === "" ? "Empty content." : text}
+
+
+ ))}
- ) - ) : lazySource || renderablePatch?.kind === "files" ? ( -
-
{ - const composedPath = event.nativeEvent.composedPath?.() ?? []; - for (const node of composedPath) { - if (!(node instanceof HTMLElement)) continue; - // Header buttons own their clicks; bubbling a chevron click would toggle twice. - if (node instanceof HTMLButtonElement || node instanceof HTMLAnchorElement) { + ) : !renderablePatch && !lazySource ? ( + isLoadingSelectedPatch ? ( + + ) : ( +
+

+ {hasNoNetChanges + ? "No net changes in this selection." + : "No patch available for this selection."} +

+
+ ) + ) : lazySource || renderablePatch?.kind === "files" ? ( +
+
{ + const composedPath = event.nativeEvent.composedPath?.() ?? []; + for (const node of composedPath) { + if (!(node instanceof HTMLElement)) continue; + // Header controls own their clicks; bubbling a chevron click would toggle twice. + if ( + node instanceof HTMLButtonElement || + node instanceof HTMLAnchorElement || + node instanceof HTMLLabelElement + ) { + return; + } + } + const title = composedPath.find( + (node): node is HTMLElement => + node instanceof HTMLElement && node.hasAttribute("data-title"), + ); + const filePath = title?.textContent; + if (filePath) { + openDiffFile(filePath); return; } - } - const title = composedPath.find( - (node): node is HTMLElement => - node instanceof HTMLElement && node.hasAttribute("data-title"), - ); - const filePath = title?.textContent; - if (filePath) { - openDiffFile(filePath); - return; - } - const header = composedPath.find( - (node): node is HTMLElement => - node instanceof HTMLElement && node.hasAttribute("data-diffs-header"), - ); - const headerFilePath = header?.querySelector("[data-title]")?.textContent; - if (!headerFilePath) return; - const file = codeViewFiles.find( - (candidate) => candidate.filePath === headerFilePath, - ); - if (file) toggleDiffFileCollapsed(file.fileKey); - }} - onContextMenuCapture={(event) => { - const composedPath = event.nativeEvent.composedPath?.() ?? []; - const title = composedPath.find( - (node): node is HTMLElement => - node instanceof HTMLElement && node.hasAttribute("data-title"), - ); - const filePath = title?.textContent?.trim(); - if (!filePath) return; - event.preventDefault(); - onFileContextMenu( - { - environmentId: activeThread?.environmentId ?? null, - filePath, - workspaceRoot: activeCwd, - repositoryRoot: activeRepositoryRoot, - }, - event, - ); - }} - > - { - const path = resolveFileDiffPath(fileDiff); - const state = fileStates.get(path); - return ( - + const header = composedPath.find( + (node): node is HTMLElement => + node instanceof HTMLElement && node.hasAttribute("data-diffs-header"), + ); + const headerFilePath = header?.querySelector("[data-title]")?.textContent; + if (!headerFilePath) return; + const file = codeViewFiles.find( + (candidate) => candidate.filePath === headerFilePath, ); + if (file) review.toggleFileCollapsed(file.fileKey); }} - {...(lazySource - ? { - unsafeCSSExtra: - "[data-additions-count], [data-deletions-count] { display: none; }", - renderHeaderMetadata: (fileDiff: FileDiffMetadata) => { - const stat = fileStats.get(resolveFileDiffPath(fileDiff)); - return stat ? ( + onContextMenuCapture={(event) => { + const composedPath = event.nativeEvent.composedPath?.() ?? []; + const title = composedPath.find( + (node): node is HTMLElement => + node instanceof HTMLElement && node.hasAttribute("data-title"), + ); + const filePath = title?.textContent?.trim(); + if (!filePath) return; + event.preventDefault(); + onFileContextMenu( + { + environmentId: activeThread?.environmentId ?? null, + filePath, + workspaceRoot: activeCwd, + repositoryRoot: activeRepositoryRoot, + }, + event, + ); + }} + > + { + const path = resolveFileDiffPath(fileDiff); + const state = fileStates.get(path); + return ( + + ); + }} + {...(lazySource + ? { + unsafeCSSExtra: + "[data-additions-count], [data-deletions-count] { display: none; }", + } + : {})} + renderHeaderMetadata={(fileDiff, fileKey) => { + const path = resolveFileDiffPath(fileDiff); + const stat = lazySource ? fileStats.get(path) : undefined; + return ( + <> + {stat ? ( - ) : null; - }, - } - : {})} - onRevealSearchMatch={unfoldDiffFile} - renderHeaderPrefix={(fileDiff, fileKey, collapsed) => { - return ( + ) : null} + {isFilePending(fileDiff) ? null : ( + { + setFileViewed(fileDiff, viewed); + setDiffFileCollapsed(fileKey, viewed); + }} + /> + )} + + ); + }} + onRevealSearchMatch={unfoldDiffFile} + renderHeaderPrefix={(fileDiff, fileKey, collapsed) => ( requestFile(resolveFileDiffPath(fileDiff)), - } + onToggle={review.toggleFileCollapsed} + {...(isFilePending(fileDiff) && !collapsedDiffFileKeys.has(fileKey) + ? { load: () => requestFile(resolveFileDiffPath(fileDiff)) } : {})} /> - ); - }} - options={{ - diffStyle: diffLayout === "split" ? "split" : "unified", - lineDiffType: "none", - overflow: wordWrap ? "wrap" : "scroll", - theme: resolveDiffThemeName(resolvedTheme), - preferredHighlighter: PREFERRED_HIGHLIGHTER, - themeType: resolvedTheme as DiffThemeType, - stickyHeaders: true, - onPostRender: flushCodeView, - ...(currentLoadDiffFiles ? { loadDiffFiles } : {}), - }} - /> -
- {fileTreeOpen ? ( - - - - ) : null} -
- ) : ( -
-
-

- {renderablePatch?.kind === "raw" ? renderablePatch.reason : null} -

- 
-                    {renderablePatch?.kind === "raw" ? renderablePatch.text : null}
-
+
+ {fileTreeOpen ? ( + + + + ) : null}
-
- )} -
- - )} + ) : ( +
+
+

+ {renderablePatch?.kind === "raw" ? renderablePatch.reason : null} +

+ 
+                      {renderablePatch?.kind === "raw" ? renderablePatch.text : null}
+
+
+
+ )} +
+ + )} + ); } diff --git a/apps/web/src/components/DiffPanelShell.tsx b/apps/web/src/components/DiffPanelShell.tsx index 456f70516b23b..82459c019bf98 100644 --- a/apps/web/src/components/DiffPanelShell.tsx +++ b/apps/web/src/components/DiffPanelShell.tsx @@ -1,4 +1,4 @@ -import type { ReactNode } from "react"; +import type { ReactNode, Ref } from "react"; import { isElectron } from "~/env"; import { cn } from "~/lib/utils"; @@ -18,17 +18,25 @@ function getDiffPanelHeaderRowClassName(mode: DiffPanelMode) { ); } -export function DiffPanelShell(props: { +export function DiffPanelShell({ + ref, + ...props +}: { mode: DiffPanelMode; header: ReactNode; children: ReactNode; + ref?: Ref; }) { const shouldUseDragRegion = isElectron && props.mode !== "sheet" && props.mode !== "embedded"; return ( + // Focusable so a click anywhere in the panel, header included, gives it the review keys.
{ + const composerKeys: string[] = []; + await using _screen = await render( + <> + +
+ , + options, + ); + + await userEvent.keyboard("a "); + + expect(composerKeys).toEqual([]); +}); + it("leaves a non-Latin launcher letter to the launcher by its physical key", async () => { const composerKeys: string[] = []; const received: string[] = []; diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx index 3ae6c1d524651..00385422f996f 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx @@ -1,6 +1,11 @@ -import type { CodeViewDiffItem, FileDiffMetadata, SelectedLineRange } from "@pierre/diffs"; +import type { + CodeViewDiffItem, + CodeViewLineSelection, + FileDiffMetadata, + SelectedLineRange, +} from "@pierre/diffs"; import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; -import { StrictMode, type ComponentProps } from "react"; +import { StrictMode, useState, type ComponentProps } from "react"; import { describe, expect, it, vi } from "vite-plus/test"; import { act, render } from "../../../test/render"; @@ -75,24 +80,42 @@ function file( }; } +function SelectableCodeView(props: { + files: ReturnType[]; + codeViewKey: string; + pendingFiles: FileDiffMetadata[]; + onDraftClose?: () => void; +}) { + const [selectedLines, setSelectedLines] = useState(null); + return ( + null} + renderHeaderPrefix={() => null} + onRevealSearchMatch={() => {}} + /> + ); +} + function view( files: ReturnType[], codeViewKey = "initial", pendingFiles: FileDiffMetadata[] = [], + onDraftClose?: () => void, ) { return ( - null} - renderHeaderPrefix={() => null} - onRevealSearchMatch={() => {}} + {...(onDraftClose ? { onDraftClose } : {})} /> ); @@ -160,4 +183,23 @@ describe("diff comment drafts", () => { ]); expect(container.querySelector("p")?.textContent).toBe("Keep this draft"); }); + + it("hands focus back when a refresh drops a draft's file, unless focus moved on", async () => { + using _drafts = setup(); + const onDraftClose = vi.fn(); + const shown = file("shown.ts"); + await using screen = await render(view([shown], "initial", [], onDraftClose), { container }); + await click("Comment on shown.ts"); + editor().focus(); + await screen.rerender(view([], "dropped", [], onDraftClose)); + expect(onDraftClose).toHaveBeenCalledOnce(); + + await screen.rerender(view([shown], "back", [], onDraftClose)); + await click("Comment on shown.ts"); + const elsewhere = document.createElement("input"); + container.append(elsewhere); + elsewhere.focus(); + await screen.rerender(view([], "moved", [], onDraftClose)); + expect(onDraftClose).toHaveBeenCalledOnce(); + }); }); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index f5a0da4b1a903..fed665dd8b607 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -2,15 +2,24 @@ import type { AnnotationSide, CodeViewDiffItem, CodeViewItem, + CodeViewLineSelection, DiffLineAnnotation, FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; import type { CodeViewHandle, CodeViewProps } from "@pierre/diffs/react"; import type { EnvironmentId, ProjectId, ScopedThreadRef } from "@t3tools/contracts"; -import { useState, type ReactNode, type Ref } from "react"; +import { + useEffect, + useEffectEvent, + useImperativeHandle, + useState, + type ReactNode, + type Ref, +} from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; +import { useStableCallback } from "~/hooks/useStableCallback"; import { fnv1a32, resolveFileDiffPath, resolveFileDiffPreviousPath } from "~/lib/diffRendering"; import { buildDiffReviewComment, @@ -36,6 +45,10 @@ interface DiffCommentAnnotationGroup { type DiffCommentLineAnnotation = DiffLineAnnotation; export type AnnotatableCodeViewHandle = CodeViewHandle; +export interface AnnotatableCodeViewReview { + /** Opens a comment draft on the range; false while another draft is open. */ + beginComment(fileKey: string, range: SelectedLineRange): boolean; +} const EMPTY_REVIEW_COMMENTS: ReadonlyArray = []; function annotationSide(range: SelectedLineRange): AnnotationSide { @@ -88,11 +101,16 @@ interface AnnotatableCodeViewProps { composerDraftTarget: ScopedThreadRef | DraftId; options: StyledDiffCodeViewOptions; viewerRef?: Ref; + reviewRef?: Ref; + selectedLines: CodeViewLineSelection | null; + onSelectedLinesChange: (selection: CodeViewLineSelection | null) => void; + onDraftClose?: () => void; + commentShortcutLabel?: string | null; onScroll?: CodeViewProps["onScroll"]; className?: string; renderCodeViewFooter?: () => ReactNode; unsafeCSSExtra?: string; - renderHeaderMetadata?: (fileDiff: FileDiffMetadata) => ReactNode; + renderHeaderMetadata?: (fileDiff: FileDiffMetadata, fileKey: string) => ReactNode; renderHeaderFilenameSuffix: (fileDiff: FileDiffMetadata) => ReactNode; renderHeaderPrefix: ( fileDiff: FileDiffMetadata, @@ -102,10 +120,6 @@ interface AnnotatableCodeViewProps { onRevealSearchMatch: (fileKey: string) => void; } -interface DiffSelectionContext { - item: CodeViewItem; -} - export function AnnotatableCodeView({ environmentId, projectId, @@ -117,6 +131,11 @@ export function AnnotatableCodeView({ composerDraftTarget, options, viewerRef, + reviewRef, + selectedLines, + onSelectedLinesChange, + onDraftClose, + commentShortcutLabel, onScroll, className, renderCodeViewFooter, @@ -131,10 +150,6 @@ export function AnnotatableCodeView({ const reviewComments = useComposerDraftStore( (store) => store.getComposerDraft(composerDraftTarget)?.reviewComments ?? EMPTY_REVIEW_COMMENTS, ); - const [selectedLines, setSelectedLines] = useState<{ - id: string; - range: SelectedLineRange; - } | null>(null); const [draft, setDraft] = useState<{ fileKey: string; filePath: string; @@ -142,6 +157,14 @@ export function AnnotatableCodeView({ annotation: DiffCommentLineAnnotation; } | null>(null); const [draftText, setDraftText] = useState(""); + const [droppedDraftCount, setDroppedDraftCount] = useState(0); + const draftDropped = useEffectEvent(() => { + if (document.activeElement === document.body) onDraftClose?.(); + }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a refresh that drops the draft's file unmounts its focused textarea, and focus must not fall to the page + useEffect(() => { + if (droppedDraftCount > 0) draftDropped(); + }, [droppedDraftCount]); const filesByKey = new Map(files.map((file) => [file.fileKey, file])); if ( @@ -156,7 +179,7 @@ export function AnnotatableCodeView({ ) { setDraft(null); setDraftText(""); - setSelectedLines(null); + setDroppedDraftCount((count) => count + 1); } const items: CodeViewDiffItem[] = files.map( ({ fileDiff, filePath, fileKey, fileVersion, collapsed }) => { @@ -198,12 +221,17 @@ export function AnnotatableCodeView({ }, ); + const closeDraft = () => { + setDraft(null); + setDraftText(""); + onDraftClose?.(); + }; + const removeEntry = (entryId: string) => { - setSelectedLines(null); if (draft?.annotation.metadata.entries.some((entry) => entry.id === entryId)) { - setDraft(null); - setDraftText(""); + closeDraft(); } else { + onSelectedLinesChange(null); removeReviewComment(composerDraftTarget, entryId); } }; @@ -222,32 +250,28 @@ export function AnnotatableCodeView({ text, }); if (comment) addReviewComment(composerDraftTarget, comment); - setSelectedLines(null); - setDraft(null); - setDraftText(""); + closeDraft(); }; - const beginComment = (range: SelectedLineRange | null, context: DiffSelectionContext) => { - if (!range) return; - const item = context.item; - if (item.type !== "diff") return; + const beginComment = (fileKey: string, range: SelectedLineRange) => { + const file = filesByKey.get(fileKey); + if (draft !== null || !file) return false; const id = nextFileCommentId(); - const filePath = resolveFileDiffPath(item.fileDiff); const comment = buildDiffReviewComment({ id, sectionId, sectionTitle, - filePath, - fileDiff: item.fileDiff, + filePath: file.filePath, + fileDiff: file.fileDiff, range, text: "", }); - if (!comment) return; + if (!comment) return false; setDraftText(""); setDraft({ - fileKey: item.id, - filePath, - previousFilePath: resolveFileDiffPreviousPath(item.fileDiff), + fileKey, + filePath: file.filePath, + previousFilePath: resolveFileDiffPreviousPath(file.fileDiff), annotation: { side: annotationSide(range), lineNumber: range.end, @@ -256,11 +280,24 @@ export function AnnotatableCodeView({ }, }, }); + return true; }; + useImperativeHandle(reviewRef, () => ({ beginComment })); + const onGutterUtilityClick = useStableCallback( + ( + range: SelectedLineRange | null, + { item }: { item: CodeViewItem }, + ) => { + if (range) beginComment(item.id, range); + }, + ); return ( key={codeViewKey} + gutterUtilityLabel={ + commentShortcutLabel ? `Add comment (${commentShortcutLabel})` : "Add comment" + } {...(viewerRef ? { viewerRef } : {})} {...(onScroll ? { onScroll } : {})} {...(className ? { className } : {})} @@ -268,18 +305,18 @@ export function AnnotatableCodeView({ {...(renderHeaderMetadata ? { renderHeaderMetadata: (item: CodeViewItem) => - item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, + item.type === "diff" ? renderHeaderMetadata(item.fileDiff, item.id) : null, } : {})} {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} items={items} selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} + onSelectedLinesChange={onSelectedLinesChange} options={{ ...options, enableGutterUtility: draft === null, enableLineSelection: draft === null, - onGutterUtilityClick: beginComment, + onGutterUtilityClick, onRevealSearchMatch: (item) => onRevealSearchMatch(item.id), }} renderHeaderFilenameSuffix={(item) => diff --git a/apps/web/src/components/diffs/DiffReviewControls.tsx b/apps/web/src/components/diffs/DiffReviewControls.tsx new file mode 100644 index 0000000000000..0bec938b90055 --- /dev/null +++ b/apps/web/src/components/diffs/DiffReviewControls.tsx @@ -0,0 +1,117 @@ +import { CheckIcon, FileIcon } from "lucide-react"; + +import { Checkbox } from "../ui/checkbox"; +import { Toggle } from "../ui/toggle-group"; +import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; +import { useIsDiffReviewCursorFile } from "./useDiffReviewKeyboard"; + +const withShortcut = (label: string, shortcut: string | null) => + shortcut ? `${label} (${shortcut})` : label; + +/** The viewed count; once every file is viewed it reads as a finished review. */ +export function DiffViewedCount({ viewed, total }: { viewed: number; total: number }) { + if (total === 0 || viewed < total) { + return ( + <> + {viewed} / {total} + + ); + } + return ( + + + All {total} + + ); +} + +/** Outlines the file header under the review cursor; render it in the header prefix. */ +export function DiffCursorOutline({ filePath }: { filePath: string }) { + if (!useIsDiffReviewCursorFile(filePath)) return null; + return ( + // The sticky header is the nearest positioned box, so this outlines the whole header. + + ); +} + +export function DiffViewedToggle({ + filePath, + viewed, + stale = false, + shortcutLabel, + onViewedChange, +}: { + filePath: string; + viewed: boolean; + /** The file changed after it was marked viewed. */ + stale?: boolean; + shortcutLabel: string | null; + onViewedChange: (viewed: boolean, extendRange: boolean) => void; +}) { + const action = withShortcut(viewed ? "Mark as not viewed" : "Mark as viewed", shortcutLabel); + return ( + + event.stopPropagation()} + /> + } + > + + onViewedChange( + next === true, + "shiftKey" in details.event && details.event.shiftKey === true, + ) + } + /> + {stale ? ( + Changed + ) : ( + Viewed + )} + + + {stale ? `This file has been pushed to since you marked it viewed. ${action}` : action} + + + ); +} + +export function DiffSingleFileToggle({ + singleFile, + shortcutLabel, + onToggle, +}: { + singleFile: boolean; + shortcutLabel: string | null; + onToggle: () => void; +}) { + const label = singleFile ? "Show all files" : "Show one file at a time"; + return ( + + + } + > + + + {withShortcut(label, shortcutLabel)} + + ); +} diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.tsx index a64e8d887d1f4..cf7a1244cf485 100644 --- a/apps/web/src/components/diffs/StyledDiffCodeView.tsx +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -7,10 +7,12 @@ import { type UncontrolledCodeViewProps, } from "@pierre/diffs/react"; /* oxlint-enable eslint/no-restricted-imports */ -import type { Ref } from "react"; +import { type Ref, useState } from "react"; import { DIFF_SURFACE_THEME_UNSAFE_CSS } from "~/lib/diffRendering"; import { DiffWorkerPoolProvider } from "../DiffWorkerPoolProvider"; +import { Tooltip, TooltipPopup } from "../ui/tooltip"; +import { keepShallowEqual, keepUnchangedItems, useKeepUnchanged } from "./keepUnchanged"; export const DIFF_VIEW_UNSAFE_CSS = `${DIFF_SURFACE_THEME_UNSAFE_CSS} /* A 1fr track's automatic minimum grows with the currently mounted lines. In split @@ -226,6 +228,8 @@ type StyledDiffCodeViewProps = ( readonly options?: StyledDiffCodeViewOptions; readonly viewerRef?: Ref>; readonly unsafeCSSExtra?: string; + /** Tooltip for the gutter's "+" button while it is enabled. */ + readonly gutterUtilityLabel?: string; }; export function StyledDiffCodeView({ @@ -233,37 +237,74 @@ export function StyledDiffCodeView({ viewerRef, className, unsafeCSSExtra, + gutterUtilityLabel, ...props }: StyledDiffCodeViewProps) { + const [hoveredUtilityButton, setHoveredUtilityButton] = useState(null); + // The viewer redraws for a new options object or item list, so keep them while nothing changed. + const keptOptions = useKeepUnchanged(options ?? {}, keepShallowEqual); + const keptItems = useKeepUnchanged(props.items, (previous, next) => + previous && next ? keepUnchangedItems(previous, next) : next, + ); + const viewProps = + props.items === undefined || keptItems === undefined ? props : { ...props, items: keptItems }; + const view = ( + + {...viewProps} + {...(viewerRef ? { ref: viewerRef } : {})} + className={ + className + ? `diff-render-surface [--code-background:var(--background)] outline-none ${className}` + : "diff-render-surface [--code-background:var(--background)] outline-none" + } + options={{ + ...keptOptions, + unsafeCSS: unsafeCSSExtra + ? `${DIFF_VIEW_UNSAFE_CSS}\n${unsafeCSSExtra}` + : DIFF_VIEW_UNSAFE_CSS, + itemMetrics: { + diffHeaderHeight: 32, + hunkSeparatorHeight: 24, + // Zero both spacing and paddingTop; Pierre's expanded-file fallback otherwise adds an 8px header + // gap. + spacing: 0, + paddingTop: 0, + // Include Pierre's 8px --diffs-gap-fallback in metrics or each expanded file loses a scrollable + // row. + paddingBottom: 8, + }, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + }} + /> + ); return ( - - {...props} - {...(viewerRef ? { ref: viewerRef } : {})} - className={ - className - ? `diff-render-surface [--code-background:var(--background)] outline-none ${className}` - : "diff-render-surface [--code-background:var(--background)] outline-none" - } - options={{ - ...options, - unsafeCSS: unsafeCSSExtra - ? `${DIFF_VIEW_UNSAFE_CSS}\n${unsafeCSSExtra}` - : DIFF_VIEW_UNSAFE_CSS, - itemMetrics: { - diffHeaderHeight: 32, - hunkSeparatorHeight: 24, - // Zero both spacing and paddingTop; Pierre's expanded-file fallback otherwise adds an 8px header - // gap. - spacing: 0, - paddingTop: 0, - // Include Pierre's 8px --diffs-gap-fallback in metrics or each expanded file loses a scrollable - // row. - paddingBottom: 8, - }, - layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, - }} - /> + {gutterUtilityLabel ? ( + // The viewer draws its "+" button inside a shadow root, so the tooltip is anchored from here. +
+ setHoveredUtilityButton( + event.nativeEvent + .composedPath() + .find( + (node): node is HTMLElement => + node instanceof HTMLElement && node.hasAttribute("data-utility-button"), + ) ?? null, + ) + } + onPointerLeave={() => setHoveredUtilityButton(null)} + > + + + {gutterUtilityLabel} + + + {view} +
+ ) : ( + view + )} ); } diff --git a/apps/web/src/components/diffs/diffReviewCursor.test.ts b/apps/web/src/components/diffs/diffReviewCursor.test.ts new file mode 100644 index 0000000000000..ad2a561f79366 --- /dev/null +++ b/apps/web/src/components/diffs/diffReviewCursor.test.ts @@ -0,0 +1,200 @@ +import { parseDiffFromFile } from "@pierre/diffs"; +import { describe, expect, it } from "vite-plus/test"; + +import { + adjacentFileKey, + adjacentUnviewedFileKey, + cursorFromSelectedRange, + cursorSelectedRange, + type DiffRow, + fileScrollAlign, + fileToStartFrom, + nearestGapExpansion, + stepRow, + topToBottomRange, + visibleDiffRows, +} from "./diffReviewCursor"; + +const lines = (count: number, edit: (line: number) => string) => + Array.from({ length: count }, (_, index) => `${edit(index + 1)}\n`).join(""); + +// Changes lines 8 and 22 of 30: hunks keep 4 context lines, leaving hidden gaps of 3 lines +// above, 5 between, and 4 after. +const fileDiff = parseDiffFromFile( + { name: "a.ts", contents: lines(30, (line) => `line ${line}`) }, + { + name: "a.ts", + contents: lines(30, (line) => (line === 8 || line === 22 ? `changed ${line}` : `line ${line}`)), + }, +); +const NO_EXPANSIONS = new Map(); +const unified = visibleDiffRows(fileDiff, "unified", NO_EXPANSIONS); +const split = visibleDiffRows(fileDiff, "split", NO_EXPANSIONS); +const rowAt = (rows: ReadonlyArray, lineNumber: number, side = "additions") => + rows.find((row) => row.lineNumber === lineNumber && row.side === side)!; + +describe("visibleDiffRows", () => { + it("lists deletions and additions as separate unified rows and paired split rows", () => { + expect(unified.map((row) => `${row.side[0]}${row.lineNumber}`)).toEqual( + ["a4", "a5", "a6", "a7", "d8", "a8", "a9", "a10", "a11", "a12"].concat([ + "a18", + "a19", + "a20", + "a21", + "d22", + "a22", + "a23", + "a24", + "a25", + "a26", + ]), + ); + expect(split.map((row) => `${row.side[0]}${row.lineNumber}`)).toEqual( + ["a4", "a5", "a6", "a7", "a8", "a9", "a10", "a11", "a12"].concat([ + "a18", + "a19", + "a20", + "a21", + "a22", + "a23", + "a24", + "a25", + "a26", + ]), + ); + expect(split[4]).toMatchObject({ deletionLineNumber: 8, additionLineNumber: 8 }); + expect(unified[0]).toMatchObject({ hunkIndex: 0, hiddenBefore: 3 }); + expect(rowAt(unified, 18)).toMatchObject({ hunkIndex: 1, hiddenBefore: 5 }); + expect(unified.at(-1)).toMatchObject({ hiddenAfter: 4 }); + }); + + it("includes expanded context and shrinks the remaining gap", () => { + const rows = visibleDiffRows(fileDiff, "unified", new Map([[1, { fromStart: 2, fromEnd: 0 }]])); + expect(rowAt(rows, 13)).toMatchObject({ hunkIndex: 1, hiddenBefore: 0 }); + expect(rowAt(rows, 14)).toBeDefined(); + expect(rowAt(rows, 18)).toMatchObject({ hiddenBefore: 3 }); + expect(nearestGapExpansion(fileDiff, rows, rowAt(rows, 12), "below")).toEqual({ + hunkIndex: 1, + direction: "up", + }); + }); +}); + +describe("stepRow", () => { + it("stops at the first and last row", () => { + expect(stepRow(unified, unified[0]!, -1)).toBe(unified[0]); + expect(stepRow(unified, unified[0]!, 1)).toBe(unified[1]); + expect(stepRow(unified, unified.at(-1)!, 1)).toBe(unified.at(-1)); + }); + + it("finds a context row from either side", () => { + expect(stepRow(split, { lineNumber: 4, side: "deletions" }, 1)).toBe(split[1]); + }); +}); + +describe("nearestGapExpansion", () => { + it.each([ + [5, "above", { hunkIndex: 0, direction: "down" }], + [9, "above", { hunkIndex: 0, direction: "down" }], + [5, "below", { hunkIndex: 1, direction: "up" }], + [20, "above", { hunkIndex: 1, direction: "down" }], + [20, "below", { hunkIndex: 2, direction: "up" }], + [26, "below", { hunkIndex: 2, direction: "up" }], + ] as const)("from line %i %s reveals %o", (lineNumber, toward, expansion) => { + expect(nearestGapExpansion(fileDiff, unified, rowAt(unified, lineNumber), toward)).toEqual( + expansion, + ); + }); + + it("finds nothing once the file is fully shown", () => { + const expanded = new Map( + [0, 1, 2].map((hunkIndex) => [hunkIndex, { fromStart: 1_000_000, fromEnd: 0 }]), + ); + const rows = visibleDiffRows(fileDiff, "unified", expanded); + expect(rows).toHaveLength(32); + expect(nearestGapExpansion(fileDiff, rows, rowAt(rows, 1), "below")).toBeNull(); + expect(nearestGapExpansion(fileDiff, rows, rowAt(rows, 30), "above")).toBeNull(); + }); +}); + +describe("cursor selections", () => { + const file = { fileKey: "a", filePath: "a.ts" }; + + it("orders a selection made upward top to bottom across sides", () => { + const range = cursorSelectedRange({ + mode: "select", + ...file, + anchor: { lineNumber: 11, side: "additions" }, + head: { lineNumber: 8, side: "deletions" }, + }); + expect(range).toEqual({ start: 11, side: "additions", end: 8, endSide: "deletions" }); + expect(topToBottomRange(range!, unified)).toEqual({ + start: 8, + side: "deletions", + end: 11, + endSide: "additions", + }); + }); + + it("reads a single line as line mode and a range as a selection", () => { + expect(cursorFromSelectedRange(file, { start: 6, side: "additions", end: 6 })).toEqual({ + mode: "line", + ...file, + row: { lineNumber: 6, side: "additions" }, + }); + expect( + cursorFromSelectedRange(file, { start: 8, side: "deletions", end: 9, endSide: "additions" }), + ).toEqual({ + mode: "select", + ...file, + anchor: { lineNumber: 8, side: "deletions" }, + head: { lineNumber: 9, side: "additions" }, + }); + }); +}); + +describe("file navigation", () => { + const keys = ["a", "b", "c", "d"]; + + it("starts at the first file and stops at the ends", () => { + expect(adjacentFileKey(keys, null, -1)).toBe("a"); + expect(adjacentFileKey(keys, "a", -1)).toBe("a"); + expect(adjacentFileKey(keys, "c", 1)).toBe("d"); + expect(adjacentFileKey(keys, "d", 1)).toBe("d"); + }); + + it("wraps to the next unviewed file and skips viewed ones", () => { + const viewed = new Set(["a", "d"]); + const isViewed = (key: string) => viewed.has(key); + expect(adjacentUnviewedFileKey(keys, "c", 1, isViewed)).toBe("b"); + expect(adjacentUnviewedFileKey(keys, "b", -1, isViewed)).toBe("c"); + expect(adjacentUnviewedFileKey(keys, null, 1, isViewed)).toBe("b"); + expect(adjacentUnviewedFileKey(keys, null, -1, isViewed)).toBe("c"); + expect(adjacentUnviewedFileKey(keys, "a", 1, () => true)).toBeNull(); + }); + + it("starts from the file in view when the cursor's file is off screen", () => { + const placed = keys.map((fileKey, index) => ({ fileKey, top: 10 + index * 500 })); + const view = (top: number) => ({ top, bottom: top + 400, contentEnd: 2010 }); + expect(fileToStartFrom(placed, null, view(0))).toBe("a"); + expect(fileToStartFrom(placed, null, view(1200))).toBe("c"); + expect(fileToStartFrom(placed, "a", view(1200))).toBe("c"); + expect(fileToStartFrom(placed, "b", view(1200))).toBe("c"); + expect(fileToStartFrom(placed, "c", view(1200))).toBeNull(); + expect(fileToStartFrom(placed, "d", view(1200))).toBeNull(); + expect(fileToStartFrom(placed, "d", view(1610))).toBeNull(); + }); + + it("scrolls the least to a file that fits on screen and to the header of a taller one", () => { + const placed = [ + { fileKey: "a", top: 0 }, + { fileKey: "b", top: 32 }, + { fileKey: "c", top: 400 }, + ]; + const view = { height: 300, contentEnd: 432 }; + expect(fileScrollAlign(placed, "a", view)).toBe("nearest"); + expect(fileScrollAlign(placed, "b", view)).toBe("start"); + expect(fileScrollAlign(placed, "c", view)).toBe("nearest"); + expect(fileScrollAlign(placed, "missing", view)).toBe("start"); + }); +}); diff --git a/apps/web/src/components/diffs/diffReviewCursor.ts b/apps/web/src/components/diffs/diffReviewCursor.ts new file mode 100644 index 0000000000000..714baccc93b8f --- /dev/null +++ b/apps/web/src/components/diffs/diffReviewCursor.ts @@ -0,0 +1,221 @@ +import type { + ExpansionDirections, + FileDiffMetadata, + HunkExpansionRegion, + SelectedLineRange, + SelectionSide, +} from "@pierre/diffs"; +import { iterateOverDiff } from "@pierre/diffs/utils/iterateOverDiff"; + +export interface DiffRowRef { + readonly lineNumber: number; + readonly side: SelectionSide; +} + +interface DiffCursorFile { + readonly fileKey: string; + // Lazy placeholders change identity key once loaded; the path keeps the cursor on the file. + readonly filePath: string; +} + +export type DiffReviewCursor = + | ({ readonly mode: "file" } & DiffCursorFile) + | ({ readonly mode: "line"; readonly row: DiffRowRef } & DiffCursorFile) + | ({ + readonly mode: "select"; + readonly anchor: DiffRowRef; + readonly head: DiffRowRef; + } & DiffCursorFile); + +export interface DiffRow extends DiffRowRef { + readonly hunkIndex: number; + readonly hiddenBefore: number; + readonly hiddenAfter: number; + readonly deletionLineNumber: number | null; + readonly additionLineNumber: number | null; +} + +export interface DiffGapExpansion { + readonly hunkIndex: number; + readonly direction: ExpansionDirections; +} + +/** Each row the viewer draws for the file, top to bottom, with folded lines left out. */ +export function visibleDiffRows( + fileDiff: FileDiffMetadata, + diffStyle: "unified" | "split", + expandedHunks: ReadonlyMap, +): DiffRow[] { + const rows: DiffRow[] = []; + iterateOverDiff({ + diff: fileDiff, + diffStyle, + expandedHunks: new Map(expandedHunks), + callback: ({ hunkIndex, collapsedBefore, collapsedAfter, additionLine, deletionLine }) => { + const additionLineNumber = additionLine?.lineNumber ?? null; + const deletionLineNumber = deletionLine?.lineNumber ?? null; + rows.push({ + lineNumber: additionLineNumber ?? deletionLineNumber ?? 0, + side: additionLineNumber === null ? "deletions" : "additions", + hunkIndex, + hiddenBefore: collapsedBefore, + hiddenAfter: collapsedAfter, + deletionLineNumber, + additionLineNumber, + }); + }, + }); + return rows; +} + +function findRowIndex(rows: ReadonlyArray, ref: DiffRowRef): number { + return rows.findIndex((row) => + ref.side === "additions" + ? row.additionLineNumber === ref.lineNumber + : row.deletionLineNumber === ref.lineNumber, + ); +} + +/** Moves one visible row, stopping at the first and last. A row no longer shown restarts at the top. */ +export function stepRow( + rows: ReadonlyArray, + ref: DiffRowRef, + delta: 1 | -1, +): DiffRow | null { + const index = findRowIndex(rows, ref); + if (index < 0) return rows[0] ?? null; + return rows[Math.min(rows.length - 1, Math.max(0, index + delta))] ?? null; +} + +/** + * The nearest still-hidden block on one side of the row, as the expansion that reveals the lines + * touching the row. A gap keyed `i` sits above hunk `i`; key `hunks.length` trails the last hunk. + */ +export function nearestGapExpansion( + fileDiff: FileDiffMetadata, + rows: ReadonlyArray, + ref: DiffRowRef, + toward: "above" | "below", +): DiffGapExpansion | null { + const index = findRowIndex(rows, ref); + if (index < 0) return null; + if (toward === "above") { + for (let cursor = index; cursor >= 0; cursor -= 1) { + const row = rows[cursor]; + if (row && row.hiddenBefore > 0) return { hunkIndex: row.hunkIndex, direction: "down" }; + } + return null; + } + for (let cursor = index; cursor < rows.length; cursor += 1) { + const row = rows[cursor]; + if (!row) continue; + if (cursor > index && row.hiddenBefore > 0) + return { hunkIndex: row.hunkIndex, direction: "up" }; + if (row.hiddenAfter > 0) return { hunkIndex: fileDiff.hunks.length, direction: "up" }; + } + return null; +} + +/** The cursor as a viewer selection, from where it started to where it is now. */ +export function cursorSelectedRange(cursor: DiffReviewCursor): SelectedLineRange | null { + if (cursor.mode === "file") return null; + const [from, to] = + cursor.mode === "line" ? [cursor.row, cursor.row] : [cursor.anchor, cursor.head]; + return { start: from.lineNumber, side: from.side, end: to.lineNumber, endSide: to.side }; +} + +/** Puts the range's lower row last, where a comment on it is drawn. */ +export function topToBottomRange( + range: SelectedLineRange, + rows: ReadonlyArray, +): SelectedLineRange { + const start: DiffRowRef = { lineNumber: range.start, side: range.side ?? "additions" }; + const end: DiffRowRef = { lineNumber: range.end, side: range.endSide ?? start.side }; + if (findRowIndex(rows, end) >= findRowIndex(rows, start)) return range; + return { start: end.lineNumber, side: end.side, end: start.lineNumber, endSide: start.side }; +} + +export function cursorFromSelectedRange( + file: DiffCursorFile, + range: SelectedLineRange, +): DiffReviewCursor { + const anchor: DiffRowRef = { lineNumber: range.start, side: range.side ?? "additions" }; + const head: DiffRowRef = { lineNumber: range.end, side: range.endSide ?? anchor.side }; + if (anchor.lineNumber === head.lineNumber && anchor.side === head.side) { + return { mode: "line", ...file, row: anchor }; + } + return { mode: "select", ...file, anchor, head }; +} + +export interface PlacedFile { + readonly fileKey: string; + /** Where the file's header sits in the scrolled content. */ + readonly top: number; +} + +/** + * The file whose rows cross the top of the view, to start from when the cursor's file is off + * screen or there is no cursor. Null while the cursor's file is on screen. + */ +export function fileToStartFrom( + placed: ReadonlyArray, + cursorKey: string | null, + view: { readonly top: number; readonly bottom: number; readonly contentEnd: number }, +): string | null { + const cursorIndex = placed.findIndex((file) => file.fileKey === cursorKey); + const cursorFile = placed[cursorIndex]; + if (cursorFile) { + const end = placed[cursorIndex + 1]?.top ?? view.contentEnd; + if (cursorFile.top < view.bottom && end > view.top) return null; + } + let inView = placed[0]?.fileKey ?? null; + for (const file of placed) { + if (file.top > view.top) break; + inView = file.fileKey; + } + return inView; +} + +/** + * How moving to a file scrolls: no further than it must while the whole file fits on screen, + * else to its header, since lining up a taller file's end would hide that header. + */ +export function fileScrollAlign( + placed: ReadonlyArray, + fileKey: string, + view: { readonly height: number; readonly contentEnd: number }, +): "nearest" | "start" { + const index = placed.findIndex((file) => file.fileKey === fileKey); + const file = placed[index]; + if (!file) return "start"; + const end = placed[index + 1]?.top ?? view.contentEnd; + return end - file.top <= view.height ? "nearest" : "start"; +} + +/** The neighbouring file, stopping at the ends; with no current file, the first. */ +export function adjacentFileKey( + fileKeys: ReadonlyArray, + currentKey: string | null, + delta: 1 | -1, +): string | null { + const index = currentKey === null ? -1 : fileKeys.indexOf(currentKey); + if (index < 0) return fileKeys[0] ?? null; + return fileKeys[Math.min(fileKeys.length - 1, Math.max(0, index + delta))] ?? null; +} + +/** The next file not yet viewed in the given direction, wrapping around the list. */ +export function adjacentUnviewedFileKey( + fileKeys: ReadonlyArray, + currentKey: string | null, + delta: 1 | -1, + isViewed: (fileKey: string) => boolean, +): string | null { + const count = fileKeys.length; + const currentIndex = currentKey === null ? -1 : fileKeys.indexOf(currentKey); + const start = currentIndex >= 0 ? currentIndex : delta > 0 ? -1 : count; + for (let step = 1; step <= count; step += 1) { + const key = fileKeys[(((start + delta * step) % count) + count) % count]; + if (key !== undefined && !isViewed(key)) return key; + } + return null; +} diff --git a/apps/web/src/components/diffs/keepUnchanged.test.ts b/apps/web/src/components/diffs/keepUnchanged.test.ts new file mode 100644 index 0000000000000..81ebee7aaffca --- /dev/null +++ b/apps/web/src/components/diffs/keepUnchanged.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { keepShallowEqual, keepUnchangedItems } from "./keepUnchanged"; + +describe("keepUnchangedItems", () => { + const a = { id: "a", type: "diff", version: 1 }; + const b = { id: "b", type: "diff", version: 1 }; + + it("keeps the previous list while every id, type and version matches", () => { + const previous = [a, b]; + expect(keepUnchangedItems(previous, [{ ...a }, { ...b }])).toBe(previous); + }); + + it("takes an item whose version changed and keeps the others", () => { + const changed = { ...b, version: 2 }; + const kept = keepUnchangedItems([a, b], [{ ...a }, changed]); + expect(kept[0]).toBe(a); + expect(kept[1]).toBe(changed); + }); + + it("returns a new list when items move, arrive, or leave", () => { + expect(keepUnchangedItems([a, b], [{ ...b }, { ...a }])).toEqual([b, a]); + expect(keepUnchangedItems([a], [{ ...a }, b])).toEqual([a, b]); + const previous = [a, b]; + expect(keepUnchangedItems(previous, [{ ...a }])).not.toBe(previous); + }); +}); + +describe("keepShallowEqual", () => { + const onPostRender = () => {}; + + it("keeps the previous object until a value or key changes", () => { + const previous = { theme: "dark", onPostRender }; + expect(keepShallowEqual(previous, { theme: "dark", onPostRender })).toBe(previous); + const next = { theme: "light", onPostRender }; + expect(keepShallowEqual(previous, next)).toBe(next); + const added = { theme: "dark", onPostRender, stickyHeaders: true }; + expect(keepShallowEqual(previous, added)).toBe(added); + }); +}); diff --git a/apps/web/src/components/diffs/keepUnchanged.ts b/apps/web/src/components/diffs/keepUnchanged.ts new file mode 100644 index 0000000000000..dabf25266e338 --- /dev/null +++ b/apps/web/src/components/diffs/keepUnchanged.ts @@ -0,0 +1,51 @@ +import { useState } from "react"; + +interface VersionedItem { + readonly id: string; + readonly type: string; + readonly version?: number | undefined; +} + +/** + * Keeps each item the viewer would treat as unchanged, since it ignores an item whose id, type and + * version match, and the previous list when every item is kept. A new list makes it redraw. + */ +export function keepUnchangedItems( + previous: ReadonlyArray, + next: ReadonlyArray, +): ReadonlyArray { + if (previous === next) return previous; + const previousById = new Map(previous.map((item) => [item.id, item])); + let changed = previous.length !== next.length; + const kept = next.map((item, index) => { + const old = previousById.get(item.id); + const keep = old !== undefined && old.type === item.type && old.version === item.version; + const result = keep ? old : item; + if (result !== previous[index]) changed = true; + return result; + }); + return changed ? kept : previous; +} + +export function keepShallowEqual(previous: Value, next: Value): Value { + if (previous === next) return previous; + const previousKeys = Object.keys(previous); + if (previousKeys.length !== Object.keys(next).length) return next; + for (const key of previousKeys) { + if (!Object.hasOwn(next, key) || Reflect.get(previous, key) !== Reflect.get(next, key)) { + return next; + } + } + return previous; +} + +/** The value from the last render while `keep` finds nothing changed. */ +export function useKeepUnchanged( + next: Value, + keep: (previous: Value, next: Value) => Value, +) { + const [kept, setKept] = useState(next); + const value = keep(kept, next); + if (value !== kept) setKept(value); + return value; +} diff --git a/apps/web/src/components/diffs/useCodeViewFileReveal.ts b/apps/web/src/components/diffs/useCodeViewFileReveal.ts index 60b76290fbe2f..a6bfcfacd97d4 100644 --- a/apps/web/src/components/diffs/useCodeViewFileReveal.ts +++ b/apps/web/src/components/diffs/useCodeViewFileReveal.ts @@ -65,6 +65,7 @@ export function createCodeViewFileReveal() { let detachedInstance: object | undefined; let pending: { id: string; + align: "start" | "nearest"; range?: SelectedLineRange; positionedFile?: boolean; loadingFile?: boolean; @@ -73,7 +74,8 @@ export function createCodeViewFileReveal() { const flush = () => { if (pending === null || viewer === null) return; const item = viewer.getItem(pending.id); - if (!viewer.getInstance() || item === undefined || item.collapsed === true) return; + if (!viewer.getInstance() || item === undefined) return; + if (pending.range && item.collapsed === true) return; const renderedItems = viewer.getInstance()?.getRenderedItems?.(); if (pending.range && renderedItems) { const rendered = renderedItems.find((entry) => entry.id === pending?.id); @@ -142,7 +144,7 @@ export function createCodeViewFileReveal() { viewer.scrollTo( target.range ? { type: "range", id: target.id, range: target.range, align: "center" } - : { type: "item", id: target.id, align: "start" }, + : { type: "item", id: target.id, align: target.align }, ); }; return { @@ -163,8 +165,9 @@ export function createCodeViewFileReveal() { viewer = next; flush(); }, - request(fileKey: string, range?: SelectedLineRange) { - pending = { id: fileKey, ...(range ? { range } : {}) }; + /** `nearest` leaves a file already on screen in place. */ + request(fileKey: string, range?: SelectedLineRange, align: "start" | "nearest" = "start") { + pending = { id: fileKey, align, ...(range ? { range } : {}) }; flush(); }, /** The user scrolled; a reveal still being positioned must not pull them back. */ diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx new file mode 100644 index 0000000000000..76a2cb87576a6 --- /dev/null +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -0,0 +1,192 @@ +import { parseDiffFromFile } from "@pierre/diffs"; +import { expect, it, vi } from "vite-plus/test"; +import { userEvent } from "vite-plus/test/browser"; + +import { render } from "../../../test/render"; +import { + DiffReviewCursorContext, + useDiffReviewCursorFilePath, + useDiffReviewKeyboard, + useDiffReviewSelectedLines, +} from "./useDiffReviewKeyboard"; + +vi.mock("~/state/server", async () => { + const { Atom } = await import("effect/reactivity"); + const { DEFAULT_RESOLVED_KEYBINDINGS } = await import("@t3tools/shared/keybindings"); + return { primaryServerKeybindingsAtom: Atom.make(DEFAULT_RESOLVED_KEYBINDINGS) }; +}); + +const file = { + fileKey: "a", + filePath: "a.ts", + fileDiff: parseDiffFromFile({ name: "a.ts", contents: "a\n" }, { name: "a.ts", contents: "b\n" }), + rendered: true, +}; +const otherFile = { ...file, fileKey: "b", filePath: "b.ts" }; + +function CursorFile() { + const filePath = useDiffReviewCursorFilePath(); + const onLines = useDiffReviewSelectedLines() !== null; + return filePath === null ? null :

Cursor on {onLines ? `lines of ${filePath}` : filePath}

; +} + +function Panel(props: { + folds: string[]; + clicks: string[]; + reveals?: string[]; + renders?: string[]; + hidden?: boolean; + twoFiles?: boolean; + viewedUnsupported?: boolean; + autoAdvance?: boolean; +}) { + props.renders?.push("panel"); + const { rootRef, cursorStore, singleFile, toggleFileCollapsed } = useDiffReviewKeyboard({ + scopeKey: "scope", + files: props.twoFiles ? [file, otherFile] : [file], + diffStyle: "unified", + canLoadFullFiles: false, + autoAdvanceOnViewed: props.autoAdvance ?? false, + getViewer: () => null, + isCollapsed: () => false, + setCollapsed: (fileKey) => void props.folds.push(fileKey), + isViewed: () => false, + setViewed: props.viewedUnsupported ? null : () => {}, + revealFile: (revealed) => void props.reveals?.push(revealed.fileKey), + beginComment: () => {}, + }); + return ( +
+ + + {singleFile ?

One file at a time

: null} + + + +
+ ); +} + +// router: false because the panel stand-in never navigates. +const options = { router: false } as const; + +it("takes the keys from a clicked control and drops its focus ring", async () => { + const folds: string[] = []; + const clicks: string[] = []; + await using screen = await render(, options); + await userEvent.click(screen.getByRole("button", { name: "Changes" })); + + await userEvent.keyboard("j"); + expect(document.activeElement).toBe(screen.getByTestId("panel").element()); + await userEvent.click(screen.getByRole("button", { name: "Changes" })); + await userEvent.keyboard("{Enter}"); + + expect(folds).toEqual(["a"]); + expect(clicks).toEqual(["Changes", "Changes"]); +}); + +it("leaves v alone where files cannot be marked viewed", async () => { + const folds: string[] = []; + await using screen = await render( + , + options, + ); + screen.getByTestId("panel").element().focus(); + + await userEvent.keyboard("jv"); + + expect(folds).toEqual([]); + await expect.element(screen.getByText("Cursor on a.ts")).toBeInTheDocument(); +}); + +it("leaves Enter to a control reached with Tab", async () => { + const folds: string[] = []; + const clicks: string[] = []; + await using screen = await render(, options); + screen.getByTestId("panel").element().focus(); + await userEvent.keyboard("j"); + + await userEvent.keyboard("{Tab}{Enter}"); + + expect(folds).toEqual([]); + expect(clicks).toEqual(["Changes"]); + await userEvent.keyboard("j"); + expect(document.activeElement).toBe(screen.getByTestId("panel").element()); +}); + +it("takes the keys from anywhere in the panel showing the diff", async () => { + const reveals: string[] = []; + const hiddenReveals: string[] = []; + await using screen = await render( + <> +
+ + +
+ + , + options, + ); + + await userEvent.click(screen.getByRole("button", { name: "Code" })); + await userEvent.keyboard("j"); + expect(document.activeElement).toBe(screen.getByTestId("panel").first().element()); + await userEvent.click(screen.getByRole("button", { name: "Elsewhere" })); + await userEvent.keyboard("j"); + + expect(reveals).toEqual(["a"]); + expect(hiddenReveals).toEqual([]); +}); + +it("moves the cursor without redrawing the panel", async () => { + const renders: string[] = []; + await using screen = await render(, options); + screen.getByTestId("panel").element().focus(); + const rendersBefore = renders.length; + + await userEvent.keyboard("j"); + + await expect.element(screen.getByText("Cursor on a.ts")).toBeInTheDocument(); + expect(renders).toHaveLength(rendersBefore); +}); + +it("returns to the full list when the single shown file is closed", async () => { + const folds: string[] = []; + await using screen = await render(, options); + screen.getByTestId("panel").element().focus(); + await userEvent.keyboard("f"); + await expect.element(screen.getByText("One file at a time")).toBeInTheDocument(); + + await userEvent.click(screen.getByRole("button", { name: "Collapse a.ts" })); + + await expect.element(screen.getByText("One file at a time")).not.toBeInTheDocument(); + expect(folds).toEqual(["a"]); +}); + +it("leaves a file's lines past either end", async () => { + const folds: string[] = []; + await using screen = await render(, options); + screen.getByTestId("panel").element().focus(); + await userEvent.keyboard("ji"); + await expect.element(screen.getByText("Cursor on lines of a.ts")).toBeInTheDocument(); + + await userEvent.keyboard("k"); + await expect.element(screen.getByText("Cursor on a.ts")).toBeInTheDocument(); + + await userEvent.keyboard("ijj"); + const foldsBefore = [...folds]; + await userEvent.keyboard("j"); + await expect.element(screen.getByText("Cursor on b.ts")).toBeInTheDocument(); + expect(folds).toEqual(foldsBefore); +}); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts new file mode 100644 index 0000000000000..0f3933d7cd68d --- /dev/null +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -0,0 +1,561 @@ +import { useAtomValue } from "@effect/atom-react"; +import type { + CodeViewLineScrollTarget, + CodeViewLineSelection, + FileDiffMetadata, + HunkExpansionRegion, + SelectedLineRange, +} from "@pierre/diffs"; +import type { CodeViewHandle } from "@pierre/diffs/react"; +import type { KeybindingCommand } from "@t3tools/contracts"; +import * as Schema from "effect/Schema"; +import { createContext, use, useEffect, useRef, useState, useSyncExternalStore } from "react"; + +import { useLocalStorage } from "~/hooks/useLocalStorage"; +import { useStableCallback } from "~/hooks/useStableCallback"; +import { shortcutLabelForCommand } from "~/keybindings"; +import { diffPanelHasFocus } from "~/lib/diffFocus"; +import { type CommandHandlers, registerCommands } from "~/shortcuts/dispatcher"; +import { primaryServerKeybindingsAtom } from "~/state/server"; + +import { + adjacentFileKey, + adjacentUnviewedFileKey, + cursorFromSelectedRange, + cursorSelectedRange, + type DiffReviewCursor, + type DiffRowRef, + fileScrollAlign, + fileToStartFrom, + nearestGapExpansion, + stepRow, + topToBottomRange, + visibleDiffRows, +} from "./diffReviewCursor"; + +const DIFF_REVIEW_COMMANDS = [ + "diff.nextFile", + "diff.previousFile", + "diff.nextUnviewedFile", + "diff.previousUnviewedFile", + "diff.toggleFileCollapsed", + "diff.toggleFileViewed", + "diff.toggleSingleFile", + "diff.enterLineMode", + "diff.nextLine", + "diff.previousLine", + "diff.expandBelow", + "diff.expandAbove", + "diff.comment", + "diff.startSelection", + "diff.back", +] as const satisfies ReadonlyArray; +type DiffReviewCommand = (typeof DIFF_REVIEW_COMMANDS)[number]; + +// Holding these keeps moving; holding a toggle would flip it on every repeat. +const REPEATABLE_COMMANDS: ReadonlySet = new Set([ + "diff.nextFile", + "diff.previousFile", + "diff.nextUnviewedFile", + "diff.previousUnviewedFile", + "diff.nextLine", + "diff.previousLine", + "diff.expandBelow", + "diff.expandAbove", +]); + +const NO_EXPANSIONS: ReadonlyMap = new Map(); +const SINGLE_FILE_STORAGE_KEY = "t3code.diffSingleFile"; +// The viewer's 100 lines per step pushes the cursor far off screen; a small step stays readable. +const KEYBOARD_EXPANSION_LINES = 20; + +const CONTROL_SELECTOR = + 'button, a[href], input, select, [role="button"], [role="checkbox"], [role="switch"]'; + +function isControlFocused(): boolean { + return (document.activeElement?.closest(CONTROL_SELECTOR) ?? null) !== null; +} + +/** + * The review cursor, kept outside React state: a key press redraws only what shows the cursor, + * not the panel around it. One store belongs to one diff scope. + */ +export interface DiffReviewCursorStore { + readonly get: () => DiffReviewCursor | null; + readonly getSelectedLines: () => CodeViewLineSelection | null; + readonly subscribe: (listener: () => void) => () => void; +} + +interface WritableDiffReviewCursorStore extends DiffReviewCursorStore { + readonly set: (cursor: DiffReviewCursor | null) => void; +} + +function createDiffReviewCursorStore(): WritableDiffReviewCursorStore { + let cursor: DiffReviewCursor | null = null; + let selectedLines: CodeViewLineSelection | null = null; + const listeners = new Set<() => void>(); + return { + get: () => cursor, + getSelectedLines: () => selectedLines, + subscribe: (listener) => { + listeners.add(listener); + return () => listeners.delete(listener); + }, + set: (next) => { + cursor = next; + const range = next ? cursorSelectedRange(next) : null; + selectedLines = next && range ? { id: next.fileKey, range } : null; + for (const listener of listeners) listener(); + }, + }; +} + +const NO_CURSOR_STORE: DiffReviewCursorStore = { + get: () => null, + getSelectedLines: () => null, + subscribe: () => () => {}, +}; + +export const DiffReviewCursorContext = createContext(NO_CURSOR_STORE); + +/** Whether the review cursor is on the file; matched by path, which survives lazy loading. */ +export function useIsDiffReviewCursorFile(filePath: string): boolean { + const store = use(DiffReviewCursorContext); + return useSyncExternalStore(store.subscribe, () => store.get()?.filePath === filePath); +} + +export function useDiffReviewCursorFilePath(): string | null { + const store = use(DiffReviewCursorContext); + return useSyncExternalStore(store.subscribe, () => store.get()?.filePath ?? null); +} + +/** The cursor's lines as a viewer selection; changes only when the selection does. */ +export function useDiffReviewSelectedLines(): CodeViewLineSelection | null { + const store = use(DiffReviewCursorContext); + return useSyncExternalStore(store.subscribe, store.getSelectedLines); +} + +export interface DiffReviewFile { + readonly fileKey: string; + readonly filePath: string; + readonly fileDiff: FileDiffMetadata; + /** Loaded and drawn by the viewer; placeholders can be visited but not reviewed. */ + readonly rendered: boolean; +} + +interface DiffReviewKeyboardInput { + /** The thread and diff scope; a new one starts without a cursor. */ + readonly scopeKey: string | null; + readonly files: ReadonlyArray; + readonly diffStyle: "unified" | "split"; + /** Partial diffs only expand when the full files can be loaded. */ + readonly canLoadFullFiles: boolean; + readonly autoAdvanceOnViewed: boolean; + readonly getViewer: () => CodeViewHandle | null; + readonly isCollapsed: (fileKey: string) => boolean; + readonly setCollapsed: (fileKey: string, collapsed: boolean) => void; + readonly isViewed: (file: DiffReviewFile) => boolean; + /** Null where files can't be marked viewed, which leaves the key to others. */ + readonly setViewed: ((file: DiffReviewFile, viewed: boolean) => void) | null; + /** Loads the file if needed and scrolls it into view. */ + readonly revealFile: (file: DiffReviewFile, align?: "start" | "nearest") => void; + readonly beginComment: (fileKey: string, range: SelectedLineRange) => void; +} + +/** + * Keyboard review of the Diff panel: a file cursor, a line cursor inside a file, and a line + * selection. Commands run through the shortcut dispatcher while focus is in the diff's panel. + */ +export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { + const { scopeKey, files } = input; + const [scopedStore, setScopedStore] = useState(() => ({ + scopeKey, + store: createDiffReviewCursorStore(), + })); + if (scopedStore.scopeKey !== scopeKey) { + setScopedStore({ scopeKey, store: createDiffReviewCursorStore() }); + } + const cursorStore = scopedStore.store; + const rootRef = useRef(null); + // Shows only the cursor's file, forced open; one preference for every diff view. + const [singleFile, setSingleFile] = useLocalStorage( + SINGLE_FILE_STORAGE_KEY, + false, + Schema.Boolean, + ); + const keybindings = useAtomValue(primaryServerKeybindingsAtom); + const pendingScrollRef = useRef(null); + // Chrome marks a clicked control :focus-visible on the next key press, so record how focus came. + const tabbedToRef = useRef(null); + + // Only single-file mode redraws the panel as the cursor moves, since the shown file changes. + const singleFilePath = useSyncExternalStore(cursorStore.subscribe, () => + singleFile ? (cursorStore.get()?.filePath ?? null) : null, + ); + const singleFileKey = singleFile + ? (files.find((file) => file.filePath === singleFilePath) ?? files[0])?.fileKey + : undefined; + const isCollapsed = (fileKey: string) => fileKey !== singleFileKey && input.isCollapsed(fileKey); + // Read when a key runs, not in render: the panel does not redraw for a cursor move. + const readCursor = () => { + const stored = cursorStore.get(); + const cursorFile = stored + ? (files.find((file) => file.fileKey === stored.fileKey) ?? + files.find((file) => file.filePath === stored.filePath) ?? + null) + : null; + const cursor: DiffReviewCursor | null = + stored === null || cursorFile === null + ? null + : stored.mode !== "file" && (!cursorFile.rendered || isCollapsed(cursorFile.fileKey)) + ? { mode: "file", fileKey: cursorFile.fileKey, filePath: cursorFile.filePath } + : { ...stored, fileKey: cursorFile.fileKey }; + return { cursor, cursorFile }; + }; + + const setCursor = (next: DiffReviewCursor | null) => cursorStore.set(next); + const fileCursor = (file: { fileKey: string; filePath: string }): DiffReviewCursor => ({ + mode: "file", + fileKey: file.fileKey, + filePath: file.filePath, + }); + + const fileDiffInstance = (fileKey: string) => { + const item = input + .getViewer() + ?.getInstance() + ?.getRenderedItems() + .find((candidate) => candidate.id === fileKey); + return item?.type === "diff" ? item.instance : undefined; + }; + const rowsFor = (file: DiffReviewFile) => { + const item = input.getViewer()?.getItem(file.fileKey); + return visibleDiffRows( + item?.type === "diff" ? item.fileDiff : file.fileDiff, + input.diffStyle, + fileDiffInstance(file.fileKey)?.getExpandedHunksForSearch() ?? NO_EXPANSIONS, + ); + }; + const canExpand = (file: DiffReviewFile) => !file.fileDiff.isPartial || input.canLoadFullFiles; + + const flushScroll = useStableCallback(() => { + const target = pendingScrollRef.current; + const viewer = input.getViewer(); + if (target === null || !viewer?.getInstance()) return; + const item = viewer.getItem(target.id); + // A file unfolded by the same key press is drawn on a later render. + if (!item || item.collapsed === true) return; + pendingScrollRef.current = null; + viewer.scrollTo(target); + }); + const queueScrollToRow = ( + file: DiffReviewFile, + row: DiffRowRef, + align: "nearest" | "center" = "nearest", + ) => { + pendingScrollRef.current = { + type: "line", + id: file.fileKey, + lineNumber: row.lineNumber, + side: row.side, + align, + }; + }; + const scrollToRow = (file: DiffReviewFile, row: DiffRowRef) => { + queueScrollToRow(file, row); + flushScroll(); + }; + const placeFiles = () => { + const viewer = input.getViewer()?.getInstance(); + if (!viewer) return null; + const top = viewer.getScrollTop(); + const height = viewer.getHeight(); + return { + placed: files.flatMap(({ fileKey }) => { + const fileTop = viewer.getTopForItem(fileKey); + return fileTop === undefined ? [] : [{ fileKey, top: fileTop }]; + }), + view: { top, bottom: top + height, height, contentEnd: viewer.getScrollHeight() }, + }; + }; + const moveToFile = (fileKey: string | null) => { + const file = files.find((candidate) => candidate.fileKey === fileKey); + if (!file) return; + pendingScrollRef.current = null; + setCursor(fileCursor(file)); + // Lining every file up at the top jolts the list, and the pull request header with it. + const placement = placeFiles(); + input.revealFile( + file, + placement ? fileScrollAlign(placement.placed, file.fileKey, placement.view) : "start", + ); + }; + + // After the reader scrolls away from the cursor, the file keys start from what is on screen. + const fileKeyInView = (cursorKey: string | null) => { + const placement = placeFiles(); + return placement ? fileToStartFrom(placement.placed, cursorKey, placement.view) : null; + }; + + const toggleFileCollapsed = useStableCallback((fileKey: string) => { + // Single-file mode keeps its file open, so closing that file returns to the full list. + if (fileKey === singleFileKey) setSingleFile(false); + input.setCollapsed(fileKey, !isCollapsed(fileKey)); + }); + + const runCommand = (command: DiffReviewCommand): boolean => { + const { cursor, cursorFile } = readCursor(); + const range = cursor ? cursorSelectedRange(cursor) : null; + const fileKeys = files.map((file) => file.fileKey); + const currentKey = cursorFile?.fileKey ?? null; + const isKeyViewed = (fileKey: string) => { + const file = files.find((candidate) => candidate.fileKey === fileKey); + return file !== undefined && input.isViewed(file); + }; + switch (command) { + case "diff.nextFile": + case "diff.previousFile": { + if (files.length === 0) return false; + const delta = command === "diff.nextFile" ? 1 : -1; + moveToFile(fileKeyInView(currentKey) ?? adjacentFileKey(fileKeys, currentKey, delta)); + return true; + } + case "diff.nextUnviewedFile": + case "diff.previousUnviewedFile": { + if (files.length === 0) return false; + const delta = command === "diff.nextUnviewedFile" ? 1 : -1; + const inView = fileKeyInView(currentKey); + moveToFile( + inView !== null && !isKeyViewed(inView) + ? inView + : adjacentUnviewedFileKey(fileKeys, inView ?? currentKey, delta, isKeyViewed), + ); + return true; + } + case "diff.toggleFileCollapsed": { + if (!cursorFile) return false; + toggleFileCollapsed(cursorFile.fileKey); + return true; + } + case "diff.toggleFileViewed": { + if (!cursorFile || !input.setViewed) return false; + if (!cursorFile.rendered) return true; + const viewed = !input.isViewed(cursorFile); + input.setViewed(cursorFile, viewed); + input.setCollapsed(cursorFile.fileKey, viewed); + const nextKey = + viewed && input.autoAdvanceOnViewed + ? adjacentUnviewedFileKey( + fileKeys, + cursorFile.fileKey, + 1, + (fileKey) => fileKey === cursorFile.fileKey || isKeyViewed(fileKey), + ) + : null; + moveToFile(nextKey ?? cursorFile.fileKey); + return true; + } + case "diff.toggleSingleFile": { + const file = cursorFile ?? files[0]; + if (!file) return false; + setCursor(fileCursor(file)); + setSingleFile(!singleFile); + input.revealFile(file); + return true; + } + case "diff.enterLineMode": { + const file = cursorFile ?? files[0]; + if (!file) return false; + if (!file.rendered) return true; + const firstRow = rowsFor(file)[0]; + if (!firstRow) return true; + input.setCollapsed(file.fileKey, false); + // Shift+J and Shift+K need the full file; loading it on the first one draws the new + // lines after the scroll that should follow them. A refresh mid-load rejects this + // warm-up, and expanding then loads the file itself. + if (canExpand(file)) { + fileDiffInstance(file.fileKey) + ?.prepareForEditing() + .catch(() => {}); + } + setCursor({ mode: "line", fileKey: file.fileKey, filePath: file.filePath, row: firstRow }); + scrollToRow(file, firstRow); + return true; + } + case "diff.nextLine": + case "diff.previousLine": { + if (!cursor || !cursorFile || cursor.mode === "file") return false; + const delta = command === "diff.nextLine" ? 1 : -1; + const rows = rowsFor(cursorFile); + const at = cursor.mode === "line" ? cursor.row : cursor.head; + const next = stepRow(rows, at, delta); + if (!next) return true; + if (cursor.mode === "line" && next.lineNumber === at.lineNumber && next.side === at.side) { + // Stepping past the first line returns to the file, past the last on to the next file. + const fileKey = + delta === -1 ? cursorFile.fileKey : adjacentFileKey(fileKeys, cursorFile.fileKey, 1); + if (delta === -1 || fileKey !== cursorFile.fileKey) moveToFile(fileKey); + return true; + } + const row: DiffRowRef = { lineNumber: next.lineNumber, side: next.side }; + setCursor(cursor.mode === "line" ? { ...cursor, row } : { ...cursor, head: row }); + scrollToRow(cursorFile, row); + return true; + } + case "diff.expandBelow": + case "diff.expandAbove": { + if (!cursor || !cursorFile || cursor.mode === "file") return false; + const instance = fileDiffInstance(cursorFile.fileKey); + if (!instance || !canExpand(cursorFile)) return true; + const row = cursor.mode === "line" ? cursor.row : cursor.head; + const gap = nearestGapExpansion( + cursorFile.fileDiff, + rowsFor(cursorFile), + row, + command === "diff.expandBelow" ? "below" : "above", + ); + if (!gap) return true; + instance.expandHunk(gap.hunkIndex, gap.direction, KEYBOARD_EXPANSION_LINES); + if (command === "diff.expandAbove") { + // Lines added above push the cursor down by heights the viewer only estimates until + // drawn, so centre it rather than aim for an edge it can miss. + queueScrollToRow(cursorFile, row, "center"); + } + return true; + } + case "diff.comment": { + if (!cursor || !cursorFile || !range) return false; + input.beginComment(cursorFile.fileKey, topToBottomRange(range, rowsFor(cursorFile))); + return true; + } + case "diff.startSelection": { + if (!cursor || cursor.mode === "file") return false; + const anchor = cursor.mode === "line" ? cursor.row : cursor.head; + setCursor({ + mode: "select", + fileKey: cursor.fileKey, + filePath: cursor.filePath, + anchor, + head: anchor, + }); + return true; + } + case "diff.back": { + if (!cursor || cursor.mode === "file") return false; + setCursor( + cursor.mode === "select" + ? { mode: "line", fileKey: cursor.fileKey, filePath: cursor.filePath, row: cursor.head } + : fileCursor(cursor), + ); + return true; + } + } + }; + + const handleCommand = useStableCallback((command: DiffReviewCommand, event: KeyboardEvent) => { + const handled = event.repeat && !REPEATABLE_COMMANDS.has(command) ? true : runCommand(command); + if (handled) { + event.preventDefault(); + // The viewer drops a scroll requested during a key press that reaches it. + event.stopPropagation(); + // The key now drives the diff, so a control keeping focus would show a stale focus ring. + const root = rootRef.current; + if (root && (isControlFocused() || !root.contains(document.activeElement))) { + root.focus({ preventScroll: true }); + } + } + return handled; + }); + const isLineMode = useStableCallback(() => { + const { cursor } = readCursor(); + return cursor !== null && cursor.mode !== "file"; + }); + + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- registers the review keys with the app-wide shortcut dispatcher while the panel is mounted + useEffect(() => { + const handlers: CommandHandlers = {}; + for (const command of DIFF_REVIEW_COMMANDS) { + handlers[command] = (event) => handleCommand(command, event); + } + let lastInputWasKeyboard = false; + const onPointerDown = () => { + lastInputWasKeyboard = false; + }; + const onKeyDown = () => { + lastInputWasKeyboard = true; + }; + const onFocusIn = (event: FocusEvent) => { + tabbedToRef.current = lastInputWasKeyboard ? event.target : null; + }; + window.addEventListener("pointerdown", onPointerDown, true); + window.addEventListener("keydown", onKeyDown, true); + window.addEventListener("focusin", onFocusIn, true); + // Capture phase, so the keys reach the panel before type-to-focus sends them to the composer. + const unregister = registerCommands(handlers, { + capture: true, + when: (event) => { + if (!rootRef.current || !diffPanelHasFocus(rootRef.current)) return false; + // A control reached from the keyboard keeps Enter; a clicked one hands it over. + return !( + event.key === "Enter" && + isControlFocused() && + document.activeElement === tabbedToRef.current + ); + }, + context: () => ({ diffLineMode: isLineMode() }), + }); + return () => { + unregister(); + window.removeEventListener("pointerdown", onPointerDown, true); + window.removeEventListener("keydown", onKeyDown, true); + window.removeEventListener("focusin", onFocusIn, true); + }; + }, [handleCommand, isLineMode]); + + const labelFor = (command: DiffReviewCommand, diffLineMode = false) => + shortcutLabelForCommand(keybindings, command, { context: { diffFocus: true, diffLineMode } }); + + return { + cursorStore, + singleFile, + singleFileKey, + rootRef, + flushScroll, + shortcutLabels: { + viewed: labelFor("diff.toggleFileViewed"), + comment: labelFor("diff.comment", true), + singleFile: labelFor("diff.toggleSingleFile"), + }, + toggleSingleFile: () => runCommand("diff.toggleSingleFile"), + toggleFileCollapsed, + /** Leaves single-file mode, for actions that only make sense in the full list. */ + showAllFiles: () => setSingleFile(false), + /** Puts the file cursor on a file the user picked another way, such as the file tree. */ + selectFile: (filePath: string) => { + const file = files.find((candidate) => candidate.filePath === filePath); + if (file) setCursor(fileCursor(file)); + }, + onSelectedLinesChange: (selection: CodeViewLineSelection | null) => { + if (selection === null) { + const { cursor } = readCursor(); + if (cursor && cursor.mode !== "file") setCursor(fileCursor(cursor)); + return; + } + const file = files.find((candidate) => candidate.fileKey === selection.id); + if (file) setCursor(cursorFromSelectedRange(file, selection.range)); + }, + // The draft's textarea held focus; without this it falls to the page and the keys stop. + onDraftClose: () => { + const { cursor } = readCursor(); + if (cursor?.mode === "select") { + setCursor({ + mode: "line", + fileKey: cursor.fileKey, + filePath: cursor.filePath, + row: cursor.head, + }); + } + rootRef.current?.focus({ preventScroll: true }); + }, + }; +} diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index 5b9d0af32a129..bc83561a1ad7a 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -8,6 +8,7 @@ import { type PullRequestDiffResult, } from "@t3tools/contracts"; import { expect, it, vi } from "vite-plus/test"; +import { userEvent } from "vite-plus/test/browser"; import "~/index.css"; import { __setClientSettingsForTests } from "../../hooks/useSettings"; @@ -223,7 +224,7 @@ it("keeps a revealed file open when its Viewed mark loads afterwards", async () await expect.element(screen.getByText("three()", { exact: true })).toBeVisible(); deliverViewedPaths(new Set(["a.ts"])); - await expect.element(screen.getByRole("checkbox", { name: "Viewed" })).toBeChecked(); + await expect.element(screen.getByRole("checkbox", { name: "Viewed a.ts" })).toBeChecked(); await expect .element(screen.getByRole("button", { name: "Collapse diff" })) @@ -322,6 +323,37 @@ it("never requests the diff of a selected commit the pull request no longer has" expect(mocks.diffCommits).not.toContain("gone"); }); +it("keeps an open comment on its line when the comment key is pressed again", async () => { + setup({ paged: false }); + const container = document.createElement("div"); + container.className = "flex h-[700px] w-[900px]"; + await using screen = await render( + {}} + environmentId={EnvironmentId.make("environment")} + reference={detail} + detail={detail} + selectedCommitOid={null} + onSelectedCommitChange={() => {}} + onRefresh={() => {}} + onAddToAgentSelection={() => {}} + />, + { container }, + ); + await expect.element(screen.getByText("1 file", { exact: true })).toBeVisible(); + const panel = container.querySelector("[data-diff-panel]")!; + panel.focus(); + await userEvent.keyboard("ic"); + const draft = screen.getByRole("textbox"); + await expect.element(draft).toBeVisible(); + const line = draft.element().getAttribute("aria-label"); + + panel.focus(); + await userEvent.keyboard("jjjc"); + + await expect.element(screen.getByRole("textbox")).toHaveAccessibleName(line!); +}); + it("answers a commit's diff link only once that commit's diff is shown", async () => { setup({ paged: false }); const container = document.createElement("div"); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 49eb9063c50c0..c94ca7f43751c 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -10,10 +10,11 @@ import { import type { CodeViewItem, DiffLineAnnotation, + FileDiffContentsLoader, FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; -import type { CodeViewAnnotationArgs, CodeViewDiffItem } from "@pierre/diffs/react"; +import type { CodeViewAnnotationArgs, CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; import type { EnvironmentId, PullRequestDetailView, @@ -51,11 +52,13 @@ import { useEffectEvent, useRef, useState, + type ComponentProps, type ReactNode, } from "react"; import { observeIntersection } from "~/lib/observeIntersection"; import { useLocalStorage } from "~/hooks/useLocalStorage"; +import { useStableCallback } from "~/hooks/useStableCallback"; import { useClientSettings, updateClientSettings } from "~/hooks/useSettings"; import { useRenderedAppearance } from "~/hooks/useTheme"; import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; @@ -102,11 +105,21 @@ import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; import { DiffFileTree } from "../diffs/DiffFileTree"; import { FileExplorerPane } from "../files/FileExplorerPane"; import { useCodeViewFileReveal } from "../diffs/useCodeViewFileReveal"; +import { + DiffCursorOutline, + DiffSingleFileToggle, + DiffViewedCount, + DiffViewedToggle, +} from "../diffs/DiffReviewControls"; +import { + DiffReviewCursorContext, + useDiffReviewKeyboard, + useDiffReviewSelectedLines, +} from "../diffs/useDiffReviewKeyboard"; import { useHideTestFiles } from "../diffs/useHideTestFiles"; import { diffFileTreeEntries, orderDiffFilesLikeTree } from "../diffs/diffFileTree.logic"; import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; import { Button } from "../ui/button"; -import { Checkbox } from "../ui/checkbox"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { DropdownMenu, @@ -273,6 +286,13 @@ interface FileViewedToggleState { readonly isViewed: (path: string) => boolean; readonly isStale: (path: string) => boolean; readonly setFileViewed: (fileKey: string, viewed: boolean, extendRange: boolean) => void; + readonly shortcutLabel: string | null; +} + +/** The viewer with the review cursor's lines selected; a cursor move redraws only this. */ +function ReviewCodeView(props: ComponentProps>) { + const selectedLines = useDiffReviewSelectedLines(); + return {...props} selectedLines={selectedLines} />; } // Context updates viewed toggles without invalidating memoized header portals. @@ -289,40 +309,18 @@ function FileViewedToggle({ }) { const viewedFiles = use(FileViewedToggleContext); if (viewedFiles === null || !viewedFiles.enabled) return stat; - const viewed = viewedFiles.isViewed(path); - const stale = viewedFiles.isStale(path); return ( {stat} - + + viewedFiles.setFileViewed(fileKey, viewed, extendRange) + } + /> ); } @@ -393,6 +391,7 @@ function PullRequestCodeTab({ useDiffWorkerPool(); const settings = useClientSettings((settings) => ({ diffFilesCollapsed: settings.diffFilesCollapsed, + diffAutoAdvanceOnViewed: settings.diffAutoAdvanceOnViewed, diffIgnoreWhitespace: settings.diffIgnoreWhitespace, diffLayout: settings.diffLayout, wordWrap: settings.wordWrap, @@ -413,10 +412,6 @@ function PullRequestCodeTab({ Schema.Boolean, ); const [hideTestFiles, setHideTestFiles] = useHideTestFiles(); - const [selectedLines, setSelectedLines] = useState<{ - id: string; - range: SelectedLineRange; - } | null>(null); const [draft, setDraft] = useState(null); const [threadPending, setThreadPending] = useState(false); const [orphansOpen, setOrphansOpen] = useState(false); @@ -442,7 +437,6 @@ function PullRequestCodeTab({ setStateScopeKey(scopeKey); setLastViewedFileKey(null); setDraft(null); - setSelectedLines(null); setFoldChoices(new Map()); setFoldOverride(null); setVisibleCommitCount(COMMIT_PAGE_SIZE); @@ -575,12 +569,15 @@ function PullRequestCodeTab({ const codeViewReveal = useCodeViewFileReveal(); const answeredRevealNonce = useRef(null); - const loadDiffFiles = createPullRequestDiffFileContentsLoader(getDiffFileContents, { + const currentLoadDiffFiles = createPullRequestDiffFileContentsLoader(getDiffFileContents, { environmentId, reference, commit, cacheKey: `pull-request:${referenceKey}:${detail.updatedAt}:${commit ?? "all"}`, }); + const loadDiffFiles: FileDiffContentsLoader = useStableCallback((fileDiff) => + currentLoadDiffFiles(fileDiff), + ); const completionScope = pullRequestCompletionScope(detail, environmentId, reference); const hostReview = detail.capabilities.review; const review = { @@ -790,14 +787,72 @@ function PullRequestCodeTab({ // A stale mark means the file changed since it was viewed; Fold all and Expand all outrank marks. const foldsAsViewed = (path: string) => foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); - const items: CodeViewDiffItem[] = annotatedFiles.map( - ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { - const collapsed = isFileDiffCollapsed( - fileKey, - effectiveFoldOverride, - foldChoices, - foldsAsViewed(path), - ); + const pathByFileKey = new Map(annotatedFiles.map((file) => [file.fileKey, file.path])); + const isCollapsed = (fileKey: string) => { + const path = pathByFileKey.get(fileKey); + return isFileDiffCollapsed( + fileKey, + effectiveFoldOverride, + foldChoices, + path !== undefined && foldsAsViewed(path), + ); + }; + const setCollapsed = (fileKey: string, collapsed: boolean) => + setFoldChoices((current) => withFileDiffFold(current, fileKey, collapsed)); + const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { + const range = getFileViewedRange( + annotatedFiles, + fileKey, + extendRange ? lastViewedFileKey : null, + ); + setLastViewedFileKey(fileKey); + for (const file of range) { + // A commit's diff names that commit's version of the file, not the head's. + setViewed(file.path, viewed, commit === null ? file.fileDiff.newObjectId : undefined); + } + setFoldChoices((current) => + range.reduce((next, file) => withFileDiffFold(next, file.fileKey, viewed), current), + ); + }; + const viewerHandleRef = useRef | null>(null); + const attachViewer = useStableCallback( + (handle: CodeViewHandle | null) => { + viewerHandleRef.current = handle; + codeViewReveal.attach(handle); + }, + ); + const { rootRef: reviewRootRef, ...diffReview } = useDiffReviewKeyboard({ + scopeKey, + files: annotatedFiles.map(({ fileKey, path, fileDiff }) => ({ + fileKey, + filePath: path, + fileDiff, + rendered: true, + })), + diffStyle: diffLayout === "split" ? "split" : "unified", + canLoadFullFiles: true, + autoAdvanceOnViewed: settings.diffAutoAdvanceOnViewed, + getViewer: () => viewerHandleRef.current, + isCollapsed, + setCollapsed, + isViewed: (file) => filesViewedEnabled && isFileViewed(file.filePath), + setViewed: filesViewedEnabled + ? (file, viewed) => setFileViewed(file.fileKey, viewed, false) + : null, + revealFile: (file, align) => codeViewReveal.request(file.fileKey, undefined, align), + beginComment: (fileKey, range) => { + const item = items.find((candidate) => candidate.id === fileKey); + if (item) beginComment(range, { item }); + }, + }); + + const items: CodeViewDiffItem[] = annotatedFiles + .filter( + ({ fileKey }) => + diffReview.singleFileKey === undefined || fileKey === diffReview.singleFileKey, + ) + .map(({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { + const collapsed = fileKey !== diffReview.singleFileKey && isCollapsed(fileKey); const viewedMark = filesViewedEnabled ? `e${isFileViewed(path) ? "v" : ""}${isFileViewedStale(path) ? "s" : ""}` : ""; @@ -809,8 +864,7 @@ function PullRequestCodeTab({ collapsed, version: fnv1a32(`${collapsed ? "1" : "0"}:${viewedMark}:${annotationsVersion}`), }; - }, - ); + }); const fileKeys = items.map((item) => item.id); const collapsedFileKeys = new Set( items.filter((item) => item.collapsed === true).map((item) => item.id), @@ -838,50 +892,26 @@ function PullRequestCodeTab({ ); }; - const toggleFile = (fileKey: string) => { - const path = annotatedFiles.find((file) => file.fileKey === fileKey)?.path; - const viewed = path !== undefined && foldsAsViewed(path); - setFoldChoices((current) => - withFileDiffFold( - current, - fileKey, - !isFileDiffCollapsed(fileKey, effectiveFoldOverride, current, viewed), - ), - ); - }; - - const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { - const range = getFileViewedRange( - annotatedFiles, - fileKey, - extendRange ? lastViewedFileKey : null, - ); - setLastViewedFileKey(fileKey); - for (const file of range) { - // A commit's diff names that commit's version of the file, not the head's. - setViewed(file.path, viewed, commit === null ? file.fileDiff.newObjectId : undefined); - } - setFoldChoices((current) => - range.reduce((next, file) => withFileDiffFold(next, file.fileKey, viewed), current), - ); - }; const fileViewedToggle = { enabled: filesViewedEnabled, isViewed: isFileViewed, isStale: isFileViewedStale, setFileViewed, + shortcutLabel: diffReview.shortcutLabels.viewed, }; const revealFile = (path: string, range?: SelectedLineRange) => { - const item = items.find((candidate) => resolveFileDiffPath(candidate.fileDiff) === path); - if (item === undefined) return; + const file = annotatedFiles.find((candidate) => candidate.path === path); + if (file === undefined) return; + if (range) diffReview.onSelectedLinesChange({ id: file.fileKey, range }); + else diffReview.selectFile(path); // Record the unfold even when already open, so a Viewed mark that loads later cannot fold it. - setFoldChoices((current) => withFileDiffFold(current, item.id, false)); - if (range) setSelectedLines({ id: item.id, range }); - codeViewReveal.request(item.id, range); + setCollapsed(file.fileKey, false); + codeViewReveal.request(file.fileKey, range); }; const toggleAllFiles = () => { + diffReview.showAllFiles(); setFoldOverride(areAllDiffFilesCollapsed(fileKeys, collapsedFileKeys) ? "expanded" : "folded"); setFoldChoices(new Map()); }; @@ -891,33 +921,37 @@ function PullRequestCodeTab({ (left, right) => Date.parse(right.committedDate) - Date.parse(left.committedDate), ); - const beginComment = ( - range: SelectedLineRange | null, - context: { item: CodeViewItem }, - ) => { - if (!range || !canSelectLines) return; - const item = context.item; - if (item.type !== "diff") return; - const file = files.find((candidate) => buildFileDiffRenderKey(candidate) === item.id); - if (!file) return; - // On a host without ranged comments a range collapses to its last line: one that silently - // lost its first line there would be worse than one line. - const path = resolveFileDiffPath(file); - const previousPath = resolveFileDiffPreviousPath(file); - const sourceFile = parsedSlices - .flatMap((slice) => (slice?.kind === "files" ? slice.sourceFiles : [])) - .find((candidate) => resolveFileDiffPath(candidate) === path); - const anchor = - (sourceFile && resolveDiffReviewAnchor(sourceFile, range, rangeComments)) ?? - resolveDiffReviewAnchor(file, range, rangeComments); - if (anchor === null) return; - setDraft({ - fileKey: item.id, - path, - oldPath: previousPath === path ? null : previousPath, - ...anchor, - range, - }); + const beginComment = useStableCallback( + (range: SelectedLineRange | null, context: { item: CodeViewItem }) => { + if (!range || !canSelectLines || draft !== null) return; + const item = context.item; + if (item.type !== "diff") return; + const file = files.find((candidate) => buildFileDiffRenderKey(candidate) === item.id); + if (!file) return; + // On a host without ranged comments a range collapses to its last line: one that silently + // lost its first line there would be worse than one line. + const path = resolveFileDiffPath(file); + const previousPath = resolveFileDiffPreviousPath(file); + const sourceFile = parsedSlices + .flatMap((slice) => (slice?.kind === "files" ? slice.sourceFiles : [])) + .find((candidate) => resolveFileDiffPath(candidate) === path); + const anchor = + (sourceFile && resolveDiffReviewAnchor(sourceFile, range, rangeComments)) ?? + resolveDiffReviewAnchor(file, range, rangeComments); + if (anchor === null) return; + setDraft({ + fileKey: item.id, + path, + oldPath: previousPath === path ? null : previousPath, + ...anchor, + range, + }); + }, + ); + + const closeDraft = () => { + setDraft(null); + diffReview.onDraftClose(); }; const finishSelection = ( @@ -938,8 +972,7 @@ function PullRequestCodeTab({ range: anchor.range, text, }); - setDraft(null); - setSelectedLines(null); + closeDraft(); if (comment !== null) onFinish(comment); }; @@ -966,23 +999,28 @@ function PullRequestCodeTab({ const renderHeaderPrefix = (item: CodeViewItem) => { const collapsed = item.collapsed === true; return ( - + <> + {item.type === "diff" ? ( + + ) : null} + + ); }; @@ -1009,6 +1047,10 @@ function PullRequestCodeTab({ ); }; + const onPostRender = useStableCallback(() => { + codeViewReveal.flush(); + diffReview.flushScroll(); + }); const diffViewOptions = { diffStyle: diffLayout === "split" ? ("split" as const) : ("unified" as const), lineDiffType: "none" as const, @@ -1022,7 +1064,7 @@ function PullRequestCodeTab({ enableLineSelection: canSelectLines && draft === null, onGutterUtilityClick: beginComment, onLineSelectionEnd: beginComment, - onPostRender: codeViewReveal.flush, + onPostRender, // Find may repeat requests before render; only unfold, never toggle. onRevealSearchMatch: (item: CodeViewItem) => setFoldChoices((current) => withFileDiffFold(current, item.id, false)), @@ -1159,10 +1201,7 @@ function PullRequestCodeTab({ }, } : {})} - onCancel={() => { - setDraft(null); - setSelectedLines(null); - }} + onCancel={closeDraft} onComment={(body) => { if (!canCommentOnLines) { if (onAddToAgentSelection) { @@ -1180,8 +1219,7 @@ function PullRequestCodeTab({ position: draft.position, body, }); - setDraft(null); - setSelectedLines(null); + closeDraft(); }} /> ) : null} @@ -1193,7 +1231,7 @@ function PullRequestCodeTab({ const changeIgnoreWhitespace = (next: boolean) => { setIgnoreWhitespace(next); setDraft(null); - setSelectedLines(null); + diffReview.onSelectedLinesChange(null); }; const toolbar = (
@@ -1258,7 +1296,10 @@ function PullRequestCodeTab({ {filesViewed.enabled && files.length > 0 ? ( - {filePaths.filter(isFileViewed).length} / {files.length} + {viewedFilesStore === "environment" ? `viewed in ${APP_BASE_NAME}` : "viewed"} @@ -1488,6 +1529,13 @@ function PullRequestCodeTab({ ) : null} + {annotatedFiles.length > 0 ? ( + + ) : null} {fileKeys.length > 0 ? ( { if (revealRequest === null || answeredRevealNonce.current === revealRequest.nonce) return; - if (!items.some((item) => resolveFileDiffPath(item.fileDiff) === revealRequest.path)) { + if (!annotatedFiles.some((file) => file.path === revealRequest.path)) { if (canLoadNextSlice) loadNextSlice(); return; } @@ -1541,10 +1589,7 @@ function PullRequestCodeTab({ } const key = `${scopeKey}:${revealUrl}`; if (answeredLink.current === key) return; - if ( - !pendingLinkPath || - !items.some((item) => resolveFileDiffPath(item.fileDiff) === pendingLinkPath) - ) { + if (!pendingLinkPath || !annotatedFiles.some((file) => file.path === pendingLinkPath)) { if (canLoadNextSlice) loadNextSlice(); // Every page is loaded and none has the file. else if (!pendingLinkPath && loadedSlices.length > 0 && nextCursor === null) { @@ -1621,7 +1666,13 @@ function PullRequestCodeTab({ } return ( -
+ // Focusable so a click anywhere in the tab, toolbar included, gives it the review keys. +
{toolbar} {orphanFiles.size > 0 ? (
@@ -1698,7 +1749,7 @@ function PullRequestCodeTab({ const item = items.find( (candidate) => resolveFileDiffPath(candidate.fileDiff) === filePath, ); - if (item !== undefined) toggleFile(item.id); + if (item !== undefined) diffReview.toggleFileCollapsed(item.id); return; } } @@ -1706,20 +1757,26 @@ function PullRequestCodeTab({ > {/* The viewer must own its scroll element to position virtual rows within it. */} - - className="@container h-full overflow-auto [scrollbar-gutter:stable]" - containerRef={scrollerRef} - viewerRef={codeViewReveal.attach} - items={items} - selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} - options={diffViewOptions} - renderCodeViewFooter={renderCodeViewFooter} - renderHeaderPrefix={renderHeaderPrefix} - renderHeaderMetadata={renderHeaderMetadata} - renderAnnotation={renderAnnotation} - unsafeCSSExtra={REPLACE_FILE_COUNTS_CSS} - /> + + +
{fileTreeOpen ? ( diff --git a/apps/web/src/components/settings/KeybindingsSettings.logic.ts b/apps/web/src/components/settings/KeybindingsSettings.logic.ts index 1946922f59dc4..f0ac8edfecc59 100644 --- a/apps/web/src/components/settings/KeybindingsSettings.logic.ts +++ b/apps/web/src/components/settings/KeybindingsSettings.logic.ts @@ -217,7 +217,8 @@ const KEYBINDING_GROUPS = [ { id: "threads", title: "Threads", prefixes: ["thread", "chat", "pullRequest"] }, { id: "composer", title: "Composer", prefixes: ["composer", "modelPicker"] }, { id: "terminal", title: "Terminal", prefixes: ["terminal"] }, - { id: "preview", title: "Preview & diff", prefixes: ["preview", "diff"] }, + { id: "preview", title: "Preview", prefixes: ["preview"] }, + { id: "diff", title: "Diff", prefixes: ["diff"] }, { id: "appearance", title: "Appearance", prefixes: ["theme", "appearance", "themeEditor"] }, { id: "scripts", title: "Project scripts", prefixes: ["script"] }, ] as const; diff --git a/apps/web/src/components/settings/SettingsPanels.tsx b/apps/web/src/components/settings/SettingsPanels.tsx index 633e18a567b2f..ac182a7f4e356 100644 --- a/apps/web/src/components/settings/SettingsPanels.tsx +++ b/apps/web/src/components/settings/SettingsPanels.tsx @@ -528,6 +528,9 @@ export function useSettingsRestore(onRestored?: () => void) { ? ["Diff whitespace changes"] : []), ...(settings.diffLayout !== DEFAULT_UNIFIED_SETTINGS.diffLayout ? ["Diff layout"] : []), + ...(settings.diffAutoAdvanceOnViewed !== DEFAULT_UNIFIED_SETTINGS.diffAutoAdvanceOnViewed + ? ["Auto-advance after marking viewed"] + : []), ...(settings.proactivePanelsEnabled !== DEFAULT_UNIFIED_SETTINGS.proactivePanelsEnabled ? ["Open pull requests and changes panels proactively"] : []), @@ -663,6 +666,7 @@ export function useSettingsRestore(onRestored?: () => void) { diffFilesCollapsed: DEFAULT_UNIFIED_SETTINGS.diffFilesCollapsed, diffIgnoreWhitespace: DEFAULT_UNIFIED_SETTINGS.diffIgnoreWhitespace, diffLayout: DEFAULT_UNIFIED_SETTINGS.diffLayout, + diffAutoAdvanceOnViewed: DEFAULT_UNIFIED_SETTINGS.diffAutoAdvanceOnViewed, proactivePanelsEnabled: DEFAULT_UNIFIED_SETTINGS.proactivePanelsEnabled, showSkillsInSlashMenu: DEFAULT_UNIFIED_SETTINGS.showSkillsInSlashMenu, composerRichTextEnabled: DEFAULT_UNIFIED_SETTINGS.composerRichTextEnabled, @@ -1619,6 +1623,32 @@ export function DiffsSettingsPanel() { } /> + + updateSettings({ + diffAutoAdvanceOnViewed: DEFAULT_UNIFIED_SETTINGS.diffAutoAdvanceOnViewed, + }) + } + /> + ) : null + } + control={ + + updateSettings({ diffAutoAdvanceOnViewed: Boolean(checked) }) + } + aria-label="Auto-advance after marking viewed" + /> + } + /> diff --git a/apps/web/src/components/settings/settingsSearch.ts b/apps/web/src/components/settings/settingsSearch.ts index 7d7939e7f21f9..3961368009c5c 100644 --- a/apps/web/src/components/settings/settingsSearch.ts +++ b/apps/web/src/components/settings/settingsSearch.ts @@ -723,6 +723,12 @@ export const SETTINGS_SEARCH_ITEMS = [ to: "/settings/diffs", searchTerms: ["collapsed expanded collapse expand files pull request pr code tab"], }, + { + id: "diff-auto-advance-on-viewed", + title: "Auto-advance after marking viewed", + to: "/settings/diffs", + searchTerms: ["viewed review next unviewed file jump keyboard"], + }, { id: "diff-layout", title: "Diff layout", diff --git a/apps/web/src/diffViewedStore.test.ts b/apps/web/src/diffViewedStore.test.ts new file mode 100644 index 0000000000000..03fafcfb7755b --- /dev/null +++ b/apps/web/src/diffViewedStore.test.ts @@ -0,0 +1,73 @@ +import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; +import { describe, expect, it } from "vite-plus/test"; + +import { diffViewedEntry, diffViewedFingerprint, useDiffViewedStore } from "./diffViewedStore"; + +function patchFile(index: string, hunk: string) { + return parsePatchFiles( + `diff --git a/a.ts b/a.ts\nindex ${index} 100644\n--- a/a.ts\n+++ b/a.ts\n${hunk}`, + { cacheKeyPrefix: "a.ts" }, + )[0]!.files[0]!; +} + +const WITH_WHITESPACE = "@@ -1,2 +1,2 @@\n-a = 1\n+a = 1\n-b\n+c\n"; +const WITHOUT_WHITESPACE = "@@ -2 +2 @@\n-b\n+c\n"; + +function setup() { + useDiffViewedStore.setState({ viewedByThreadKey: {} }); + return useDiffViewedStore.getState(); +} + +describe("diffViewedFingerprint", () => { + it("survives the whitespace toggle and changes with the file contents", () => { + const viewed = patchFile("1111111..2222222", WITH_WHITESPACE); + expect(diffViewedFingerprint(patchFile("1111111..2222222", WITHOUT_WHITESPACE))).toBe( + diffViewedFingerprint(viewed), + ); + expect(diffViewedFingerprint(patchFile("1111111..3333333", WITH_WHITESPACE))).not.toBe( + diffViewedFingerprint(viewed), + ); + }); + + it("falls back to the diff content without blob ids", () => { + const withoutIndex = (hunk: string) => + parsePatchFiles(`diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n${hunk}`, { + cacheKeyPrefix: "a.ts", + })[0]!.files[0]!; + expect(diffViewedFingerprint(withoutIndex(WITHOUT_WHITESPACE))).not.toBe( + diffViewedFingerprint(withoutIndex(WITH_WHITESPACE)), + ); + }); +}); + +describe("useDiffViewedStore", () => { + it("keeps one fingerprint per file, so a changed file reads as unviewed", () => { + const store = setup(); + store.setFileViewed("thread", "a.ts", "old", true); + store.setFileViewed("thread", "a.ts", "new", true); + expect(useDiffViewedStore.getState().viewedByThreadKey.thread).toEqual([ + diffViewedEntry("a.ts", "new"), + ]); + store.setFileViewed("thread", "a.ts", "new", false); + expect(useDiffViewedStore.getState().viewedByThreadKey).toEqual({}); + }); + + it("drops the oldest files and the least recently reviewed threads past the caps", () => { + const store = setup(); + for (let index = 0; index <= 2000; index += 1) { + store.setFileViewed("big", `file-${index}.ts`, "v", true); + } + const files = useDiffViewedStore.getState().viewedByThreadKey.big; + expect(files).toHaveLength(2000); + expect(files?.[0]).toBe(diffViewedEntry("file-1.ts", "v")); + + for (let index = 0; index < 100; index += 1) { + store.setFileViewed(`thread-${index}`, "a.ts", "v", true); + } + store.setFileViewed("thread-0", "b.ts", "v", true); + const threads = Object.keys(useDiffViewedStore.getState().viewedByThreadKey); + expect(threads).toHaveLength(100); + expect(threads).not.toContain("big"); + expect(threads.at(-1)).toBe("thread-0"); + }); +}); diff --git a/apps/web/src/diffViewedStore.ts b/apps/web/src/diffViewedStore.ts new file mode 100644 index 0000000000000..5c15060d742c8 --- /dev/null +++ b/apps/web/src/diffViewedStore.ts @@ -0,0 +1,64 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; +import { create } from "zustand"; +import { createJSONStorage, persist } from "zustand/middleware"; + +import { buildFileDiffContentVersion } from "./lib/diffRendering"; +import { resolveStorage } from "./lib/storage"; + +// Nothing removes a thread's entries when it goes away, so both levels drop their oldest. +const MAX_VIEWED_FILES_PER_THREAD = 2000; +const MAX_VIEWED_THREADS = 100; + +/** + * Identifies the reviewed contents of a file. Blob ids survive the whitespace toggle, which + * changes the hunks but not the files; diffs without them fall back to the rendered content. + */ +export function diffViewedFingerprint(fileDiff: FileDiffMetadata): string { + return fileDiff.prevObjectId && fileDiff.newObjectId + ? `${fileDiff.prevObjectId}..${fileDiff.newObjectId}` + : `v${buildFileDiffContentVersion(fileDiff)}`; +} + +export function diffViewedEntry(filePath: string, fingerprint: string): string { + return `${filePath}\u0000${fingerprint}`; +} + +interface DiffViewedStoreState { + /** Per thread, viewed `path\0fingerprint` entries, newest last. Threads are oldest first. */ + viewedByThreadKey: Record>; + setFileViewed: ( + threadKey: string, + filePath: string, + fingerprint: string, + viewed: boolean, + ) => void; +} + +export const useDiffViewedStore = create()( + persist( + (set) => ({ + viewedByThreadKey: {}, + setFileViewed: (threadKey, filePath, fingerprint, viewed) => + set((state) => { + const pathPrefix = diffViewedEntry(filePath, ""); + const entries = (state.viewedByThreadKey[threadKey] ?? []).filter( + (entry) => !entry.startsWith(pathPrefix), + ); + if (viewed) entries.push(diffViewedEntry(filePath, fingerprint)); + const { [threadKey]: _previous, ...others } = state.viewedByThreadKey; + const threads = Object.entries(others); + if (entries.length > 0) + threads.push([threadKey, entries.slice(-MAX_VIEWED_FILES_PER_THREAD)]); + return { viewedByThreadKey: Object.fromEntries(threads.slice(-MAX_VIEWED_THREADS)) }; + }), + }), + { + name: "t3code:diff-viewed-files:v1", + version: 1, + storage: createJSONStorage(() => + resolveStorage(typeof window !== "undefined" ? window.localStorage : undefined), + ), + partialize: (state) => ({ viewedByThreadKey: state.viewedByThreadKey }), + }, + ), +); diff --git a/apps/web/src/keybindings.test.ts b/apps/web/src/keybindings.test.ts index 3b596f838fa0d..2c8e5b149e788 100644 --- a/apps/web/src/keybindings.test.ts +++ b/apps/web/src/keybindings.test.ts @@ -984,3 +984,35 @@ describe("Usage shortcuts", () => { ); }); }); + +describe("Diff review shortcuts", () => { + it.each([ + [event({ key: "j" }), { diffFocus: true }, "diff.nextFile"], + [event({ key: "j" }), { diffFocus: true, diffLineMode: true }, "diff.nextLine"], + [event({ key: "j" }), {}, null], + [ + event({ key: "J", shiftKey: true }), + { diffFocus: true, diffLineMode: true }, + "diff.expandBelow", + ], + [ + event({ key: "V", shiftKey: true }), + { diffFocus: true, diffLineMode: true }, + "diff.startSelection", + ], + [event({ key: "v" }), { diffFocus: true, diffLineMode: true }, "diff.toggleFileViewed"], + [event({ key: "c" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], + [event({ key: " ", code: "Space" }), { diffFocus: true }, null], + [event({ key: "Enter" }), { diffFocus: true }, "diff.toggleFileCollapsed"], + [event({ key: "Escape" }), { diffFocus: true, diffLineMode: true }, "diff.back"], + [event({ key: "Escape" }), { diffFocus: true }, null], + ] as const)("resolves %o with %o to %s", (shortcut, context, command) => { + assert.strictEqual( + resolveShortcutCommand(shortcut, DEFAULT_RESOLVED_KEYBINDINGS, { + platform: "Linux", + context, + }), + command, + ); + }); +}); diff --git a/apps/web/src/lib/diffFocus.ts b/apps/web/src/lib/diffFocus.ts new file mode 100644 index 0000000000000..fba3b9ce12ea8 --- /dev/null +++ b/apps/web/src/lib/diffFocus.ts @@ -0,0 +1,14 @@ +import { isEditableFocused } from "./editableFocus"; + +/** Focus anywhere in the panel showing the diff, its header and tabs included, outside text fields. */ +export function diffPanelHasFocus(panel: Element): boolean { + const activeElement = document.activeElement; + if (activeElement === null || isEditableFocused()) return false; + const area = panel.closest("[data-workspace-area]") ?? panel; + // Hidden tabs stay mounted, so a panel's area can hold diffs that are not on screen. + return area.contains(activeElement) && panel.checkVisibility({ visibilityProperty: true }); +} + +export function isDiffFocused(): boolean { + return Array.from(document.querySelectorAll("[data-diff-panel]")).some(diffPanelHasFocus); +} diff --git a/apps/web/src/shortcuts/dispatcher.ts b/apps/web/src/shortcuts/dispatcher.ts index 9e361e825db7f..9a37897f11183 100644 --- a/apps/web/src/shortcuts/dispatcher.ts +++ b/apps/web/src/shortcuts/dispatcher.ts @@ -1,6 +1,7 @@ import type { KeybindingCommand, ResolvedKeybindingsConfig } from "@t3tools/contracts"; import { resolveShortcutCommand, type ShortcutMatchContext } from "../keybindings"; +import { isDiffFocused } from "../lib/diffFocus"; import { isEditableFocused } from "../lib/editableFocus"; import { isPreviewFocused } from "../lib/previewFocus"; import { isTerminalFocused } from "../lib/terminalFocus"; @@ -48,6 +49,7 @@ function readFocusContext(event: KeyboardEvent): Partial { return { terminalFocus: isTerminalFocused(), previewFocus: isPreviewFocused(), + diffFocus: isDiffFocused(), editableFocus: isEditableFocused(event.target), modelPickerOpen: isModelPickerOpen(), }; diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 0449990ee0b2b..0672f5ecde80c 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -69,6 +69,44 @@ match in a folded file opens it. Escape closes the search. This shortcut is not configurable. A very large uncommitted diff loads its files as you scroll, and find only searches the files loaded so far. +## Review a diff from the keyboard + +Click into the Diff panel or a pull request's Code tab to review it without the +mouse. The keys also work while a button or tab elsewhere in that panel has focus, +but not while you type in a text field. Both start on files. Press `i` to move into a file's lines, and Escape to step back out. + +On files: + +- `j` and `k` move to the next or previous file. +- `n` and `Shift+n` jump to the next or previous file you have not viewed. +- After you scroll away from the current file, these keys start from the file on screen. +- `v` marks the file viewed, which also folds it. Press it again to unmark. +- Enter folds or unfolds the file. +- `f` shows only the current file. `j`, `k`, and `n` switch which file it shows. + Folding that file, or pressing `f` again, shows every file. + +On lines: + +- `j` and `k` move one line. `k` on the first line returns to the file, and `j` + on the last line moves on to the next file. +- `Shift+j` and `Shift+k` show more of the unchanged lines below or above. +- `c` comments on the line. In a pull request, that starts a review comment. + After you send or cancel the comment, you are back on the line. +- `Shift+v` starts a selection. `j` and `k` extend it, and `c` comments on + the selected lines. +- `v` marks the file viewed and returns to files. + +In the Diff panel, viewed marks belong to this device and thread, and a file that +changes after you view it shows as not viewed again. In a pull request, `v` sets +the same marks as the Viewed checkbox; see +[Mark files as viewed](./source-control.md#mark-files-as-viewed). To jump to the next unviewed file after each +mark, turn on **Settings → Diffs → Auto-advance after marking viewed**. Once every +file is viewed, the count above the diff reads, for example, **All 5 viewed**. + +Change any of these keys in **Settings → Keybindings** under the **Diff** +commands. Showing more unchanged lines needs the full files, so it is not +available for turn and tool call diffs. + ## iPad With a hardware keyboard, use `Cmd+1` through `Cmd+9` to open the first nine @@ -124,10 +162,12 @@ Join modifiers and a key with `+`, such as `mod+shift+d` or `ctrl+l`. ## When conditions Available context keys are `terminalFocus`, `terminalOpen`, `previewFocus`, -`previewOpen`, `rightPanelOpen`, `modelPickerOpen`, `usagePageOpen`, `composerFocus`, -`draftThreadRoute`, `turnRunning`, `editableFocus`, `isWeb`, and `isDesktop`. -`rightPanelOpen` applies only to `rightPanel.new` and is true while the right panel is -open; the default `mod+t` uses it to open the panel's new-tab menu. +`previewOpen`, `rightPanelOpen`, `diffFocus`, `diffLineMode`, `modelPickerOpen`, +`usagePageOpen`, `composerFocus`, `draftThreadRoute`, `turnRunning`, `editableFocus`, +`isWeb`, and `isDesktop`. `rightPanelOpen` applies only to `rightPanel.new` and is true +while the right panel is open; the default `mod+t` uses it to open the panel's new-tab menu. +`diffFocus` is true while the Diff panel has the keyboard and no text field in it does. +`diffLineMode` is true while you are on a diff's lines or a selection. `draftThreadRoute` is true on a draft thread, including a restored draft, until it is first sent. `editableFocus` is true while a text field, the composer, or another editor has the keyboard. `isWeb` is true in a browser tab. `isDesktop` is true in the diff --git a/packages/contracts/src/keybindings.ts b/packages/contracts/src/keybindings.ts index eda5138b760b9..f7574f4099800 100644 --- a/packages/contracts/src/keybindings.ts +++ b/packages/contracts/src/keybindings.ts @@ -74,6 +74,21 @@ export const STATIC_KEYBINDING_COMMANDS = [ "view.reopenClosed", "pullRequest.copyNumber", "diff.toggle", + "diff.nextFile", + "diff.previousFile", + "diff.nextUnviewedFile", + "diff.previousUnviewedFile", + "diff.toggleFileCollapsed", + "diff.toggleFileViewed", + "diff.toggleSingleFile", + "diff.enterLineMode", + "diff.nextLine", + "diff.previousLine", + "diff.expandBelow", + "diff.expandAbove", + "diff.comment", + "diff.startSelection", + "diff.back", "preview.toggle", "preview.refresh", "preview.focusUrl", diff --git a/packages/contracts/src/settings.ts b/packages/contracts/src/settings.ts index 7bbb03595d4bb..29a78459760f4 100644 --- a/packages/contracts/src/settings.ts +++ b/packages/contracts/src/settings.ts @@ -471,6 +471,7 @@ export const ClientSettingsSchema = Schema.Struct({ confirmThreadArchive: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(false))), confirmThreadDelete: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(true))), confirmThreadUnpin: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(false))), + diffAutoAdvanceOnViewed: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(false))), diffFilesCollapsed: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(true))), diffIgnoreWhitespace: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(true))), diffLayout: DiffLayout.pipe(Schema.withDecodingDefault(Effect.succeed(DEFAULT_DIFF_LAYOUT))), @@ -2057,6 +2058,7 @@ export const ClientSettingsPatch = Schema.Struct({ confirmThreadArchive: Schema.optionalKey(Schema.Boolean), confirmThreadDelete: Schema.optionalKey(Schema.Boolean), confirmThreadUnpin: Schema.optionalKey(Schema.Boolean), + diffAutoAdvanceOnViewed: Schema.optionalKey(Schema.Boolean), diffFilesCollapsed: Schema.optionalKey(Schema.Boolean), diffIgnoreWhitespace: Schema.optionalKey(Schema.Boolean), diffLayout: Schema.optionalKey(DiffLayout), diff --git a/packages/shared/src/keybindings.ts b/packages/shared/src/keybindings.ts index 9a67264d8bd1f..d37d2cc424510 100644 --- a/packages/shared/src/keybindings.ts +++ b/packages/shared/src/keybindings.ts @@ -75,6 +75,21 @@ export const DEFAULT_KEYBINDINGS: ReadonlyArray = [ { key: "mod+w", command: "rightPanel.close", when: "!terminalFocus" }, { key: "mod+shift+t", command: "view.reopenClosed" }, { key: "mod+d", command: "diff.toggle", when: "!terminalFocus" }, + { key: "j", command: "diff.nextFile", when: "diffFocus && !diffLineMode" }, + { key: "k", command: "diff.previousFile", when: "diffFocus && !diffLineMode" }, + { key: "n", command: "diff.nextUnviewedFile", when: "diffFocus && !diffLineMode" }, + { key: "shift+n", command: "diff.previousUnviewedFile", when: "diffFocus && !diffLineMode" }, + { key: "enter", command: "diff.toggleFileCollapsed", when: "diffFocus && !diffLineMode" }, + { key: "v", command: "diff.toggleFileViewed", when: "diffFocus" }, + { key: "f", command: "diff.toggleSingleFile", when: "diffFocus && !diffLineMode" }, + { key: "i", command: "diff.enterLineMode", when: "diffFocus && !diffLineMode" }, + { key: "j", command: "diff.nextLine", when: "diffFocus && diffLineMode" }, + { key: "k", command: "diff.previousLine", when: "diffFocus && diffLineMode" }, + { key: "shift+j", command: "diff.expandBelow", when: "diffFocus && diffLineMode" }, + { key: "shift+k", command: "diff.expandAbove", when: "diffFocus && diffLineMode" }, + { key: "c", command: "diff.comment", when: "diffFocus && diffLineMode" }, + { key: "shift+v", command: "diff.startSelection", when: "diffFocus && diffLineMode" }, + { key: "escape", command: "diff.back", when: "diffFocus && diffLineMode" }, { key: "mod+shift+j", command: "preview.toggle" }, { key: "mod+r", command: "preview.refresh", when: "previewFocus" }, { key: "mod+l", command: "preview.focusUrl", when: "previewFocus" }, diff --git a/patches/@pierre__diffs@1.7.0-beta.3.patch b/patches/@pierre__diffs@1.7.0-beta.3.patch index 5704f9c799cf0..4a9f256087185 100644 --- a/patches/@pierre__diffs@1.7.0-beta.3.patch +++ b/patches/@pierre__diffs@1.7.0-beta.3.patch @@ -399,10 +399,10 @@ index 19934176d0787ae1304e771c707230d5c8837e83..298b3386eac5126a7c1ec985674e799a })); } diff --git a/package.json b/package.json -index 0a0acc4c8734a61cf2d684162914002adf74960d..935937d68f52ed96b7af6b1dbb890dbf512eb0fe 100644 +index 0a0acc4c8734a61cf2d684162914002adf74960d..57d52cd55a9d0098693806b089752d20043fe5ca 100644 --- a/package.json +++ b/package.json -@@ -60,6 +60,18 @@ +@@ -60,6 +60,22 @@ "./worker/worker-portable.js": { "types": "./dist/worker/worker-portable.d.ts", "import": "./dist/worker/worker-portable.js" @@ -418,6 +418,10 @@ index 0a0acc4c8734a61cf2d684162914002adf74960d..935937d68f52ed96b7af6b1dbb890dbf + "./utils/parsePatchFiles": { + "types": "./dist/utils/parsePatchFiles.d.ts", + "import": "./dist/utils/parsePatchFiles.js" ++ }, ++ "./utils/iterateOverDiff": { ++ "types": "./dist/utils/iterateOverDiff.d.ts", ++ "import": "./dist/utils/iterateOverDiff.js" } }, "publishConfig": { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index aaed5f657312d..aa937c0142aab 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -368,7 +368,7 @@ patchedDependencies: '@ff-labs/fff-node@0.11.0': 3be7107340e5f388114da259637828cd47f2b32eb8ae572440715c4e38cffe6e '@legendapp/list@3.7.2': 3c68607b8bb7a531873bcbe0b197d163ef1fdb4d4cf52a4ddf9d2b3804671658 '@muse-code/sdk@1.4.6': 7b495b607fc5d76b444c7c4d58821e839726a30e6f3c53564f0d59f2d5b42101 - '@pierre/diffs@1.7.0-beta.3': 4a64f63a0def94c5add9d86d70560d26d6857e1f5150133862e42e1fd77bc4e8 + '@pierre/diffs@1.7.0-beta.3': a98a6c5dd606af01a1b074c077aa6cf8cfc094d9c03e0e8dca14f350f459c59f '@react-native-ai/apple@0.12.0': 2d09870c2848d185cb05b53ed823a46e12dba519324d8dd8e584e28731990f9d '@react-native-menu/menu@2.0.0': 6872ba71b9847c5a0210b1b896923dadc3860faedfe28d9f062bcedfe786ef1d '@react-navigation/native-stack@7.21.0': 990d575444c4af2ce8218b3b7a8280fe244c03abbdb32b822a2e862f791dbf42 @@ -569,7 +569,7 @@ importers: version: 0.3.0 '@pierre/diffs': specifier: 1.7.0-beta.3 - version: 1.7.0-beta.3(patch_hash=4a64f63a0def94c5add9d86d70560d26d6857e1f5150133862e42e1fd77bc4e8)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.7.0-beta.3(patch_hash=a98a6c5dd606af01a1b074c077aa6cf8cfc094d9c03e0e8dca14f350f459c59f)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@pierre/highlights': specifier: 'catalog:' version: 0.5.0 @@ -1090,7 +1090,7 @@ importers: version: 1.8.0 '@pierre/diffs': specifier: 1.7.0-beta.3 - version: 1.7.0-beta.3(patch_hash=4a64f63a0def94c5add9d86d70560d26d6857e1f5150133862e42e1fd77bc4e8)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.7.0-beta.3(patch_hash=a98a6c5dd606af01a1b074c077aa6cf8cfc094d9c03e0e8dca14f350f459c59f)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@pierre/trees': specifier: 1.0.0-beta.6 version: 1.0.0-beta.6(@pierre/theme@2.0.0)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)(shiki@4.2.0) @@ -1506,7 +1506,7 @@ importers: dependencies: '@pierre/diffs': specifier: 1.7.0-beta.3 - version: 1.7.0-beta.3(patch_hash=4a64f63a0def94c5add9d86d70560d26d6857e1f5150133862e42e1fd77bc4e8)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.7.0-beta.3(patch_hash=a98a6c5dd606af01a1b074c077aa6cf8cfc094d9c03e0e8dca14f350f459c59f)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@t3tools/contracts': specifier: workspace:* version: link:../contracts @@ -16427,7 +16427,7 @@ snapshots: tslib: 2.8.1 webcrypto-core: 1.9.2 - '@pierre/diffs@1.7.0-beta.3(patch_hash=4a64f63a0def94c5add9d86d70560d26d6857e1f5150133862e42e1fd77bc4e8)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': + '@pierre/diffs@1.7.0-beta.3(patch_hash=a98a6c5dd606af01a1b074c077aa6cf8cfc094d9c03e0e8dca14f350f459c59f)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': dependencies: '@pierre/highlights': 0.5.0 '@pierre/patches': 0.0.1