From 0956542217ef82a1348dee69eb32f40221fc5ecf Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:10:20 -0700 Subject: [PATCH 01/22] chore(web): expose diff context expansion state from @pierre/diffs Export iterateOverDiff and add FileDiff.getExpandedHunks so the diff panel can walk the rows a file currently shows. Co-Authored-By: Claude Opus 5.5 --- patches/@pierre%2Fdiffs@1.6.1.patch | 72 +++++++++++++++++++++-------- pnpm-lock.yaml | 10 ++-- pnpm-workspace.yaml | 5 +- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/patches/@pierre%2Fdiffs@1.6.1.patch b/patches/@pierre%2Fdiffs@1.6.1.patch index c19440839908e..12cee93921e3b 100644 --- a/patches/@pierre%2Fdiffs@1.6.1.patch +++ b/patches/@pierre%2Fdiffs@1.6.1.patch @@ -1,5 +1,5 @@ diff --git a/dist/components/CodeView.d.ts b/dist/components/CodeView.d.ts -index 234ec4baa4..274ff5e32b 100644 +index 234ec4baa4a9364088917b24a64a6377d16583f8..274ff5e32b65379b30ac645f1f64bfdc96a12da2 100644 --- a/dist/components/CodeView.d.ts +++ b/dist/components/CodeView.d.ts @@ -158,6 +158,12 @@ interface CodeViewOptions extends CodeViewPassThroughOptions @@ -30,7 +30,7 @@ index 234ec4baa4..274ff5e32b 100644 * Returns the editor while this item is being edited. Partial diffs get an * editor after their files load; scrolling away or collapsing keeps it. diff --git a/dist/components/CodeView.js b/dist/components/CodeView.js -index ee5c91c5ab..7d0a2901e4 100644 +index ee5c91c5ab44742994d900a81dbd8e9c508efb49..7d0a2901e48de0d356dd784896d7f8eb4c9ad0e8 100644 --- a/dist/components/CodeView.js +++ b/dist/components/CodeView.js @@ -18,6 +18,7 @@ import { resolvePreferredHighlighter } from "../utils/resolvePreferredHighlighte @@ -330,7 +330,7 @@ index ee5c91c5ab..7d0a2901e4 100644 if (item.type === "diff") return item.instance.updateCodeViewLayout(item.item.fileDiff, item.top, void 0, item.item.annotations ?? []); diff --git a/dist/components/CodeViewSearch.js b/dist/components/CodeViewSearch.js new file mode 100644 -index 0000000000..29370adf88 +index 0000000000000000000000000000000000000000..29370adf88299d8f7572d4054334b6ce81bdaf9e --- /dev/null +++ b/dist/components/CodeViewSearch.js @@ -0,0 +1,360 @@ @@ -694,8 +694,26 @@ index 0000000000..29370adf88 +export { CodeViewSearch, collectMatches }; + +//# sourceMappingURL=CodeViewSearch.js.map +diff --git a/dist/components/FileDiff.d.ts b/dist/components/FileDiff.d.ts +index 3dcef7068e52287fedbea64a89ea4ab677f39429..2f961dcac73b478058a4cefa68775f92642974b2 100644 +--- a/dist/components/FileDiff.d.ts ++++ b/dist/components/FileDiff.d.ts +@@ -1,4 +1,4 @@ +-import { AppliedThemeStyleCache, BaseCodeOptions, BaseDiffOptions, CustomPreProperties, DiffLineAnnotation, ExpansionDirections, FileContents, FileDiffMetadata, HighlightedToken, HunkData, HunkSeparators, LineAnnotation, MaybeDiffFileInput, PostRenderPhase, PrePropertiesConfig, RenderHeaderFilenameSuffixCallback, RenderHeaderMetadataCallback, RenderHeaderPrefixCallback, RenderRange, SelectedLineRange, SelectionSide, ThemeTypes } from "../types.js"; ++import { AppliedThemeStyleCache, BaseCodeOptions, BaseDiffOptions, CustomPreProperties, DiffLineAnnotation, ExpansionDirections, FileContents, FileDiffMetadata, HighlightedToken, HunkData, HunkExpansionRegion, HunkSeparators, LineAnnotation, MaybeDiffFileInput, PostRenderPhase, PrePropertiesConfig, RenderHeaderFilenameSuffixCallback, RenderHeaderMetadataCallback, RenderHeaderPrefixCallback, RenderRange, SelectedLineRange, SelectionSide, ThemeTypes } from "../types.js"; + import { CapturedDiffSessionState, EditCompletionDecision, EditorActiveLineOptions, EditorChangeEvent, FileDiffEditCompleteEvent } from "../editor/types.js"; + import { TextDocument } from "../editor/textDocument.js"; + import { GetHoveredLineResult, GetLineIndexUtility, InteractionManager, InteractionManagerBaseOptions, SelectionWriteOptions } from "../managers/InteractionManager.js"; +@@ -206,6 +206,7 @@ declare class FileDiff { + onThemeChange(): void; + handleExpandHunk: (hunkIndex: number, direction: ExpansionDirections, expansionLineCountOverride?: number) => void; + expandHunk: (hunkIndex: number, direction: ExpansionDirections, expansionLineCountOverride?: number) => void; ++ getExpandedHunks(): ReadonlyMap; + protected loadFilesIfNecessary(): void; + /** + * In order to start an edit session, you must be using a `fileDiff` that diff --git a/dist/components/FileDiff.js b/dist/components/FileDiff.js -index 12f4f8949f..e33a5ed319 100644 +index 12f4f8949f3a1907ac9877f83937b53a966c9411..74628c567069b06ea74aa468eba1a513769200b6 100644 --- a/dist/components/FileDiff.js +++ b/dist/components/FileDiff.js @@ -1,6 +1,7 @@ @@ -707,7 +725,17 @@ index 12f4f8949f..e33a5ed319 100644 import { getHighlighterIfLoaded, getSharedHighlighter } from "../highlighter/shared_highlighter.js"; import { dequeueRender, queueRender } from "../managers/UniversalRenderingManager.js"; import { areThemesEqual } from "../utils/areThemesEqual.js"; -@@ -724,7 +725,7 @@ var FileDiff = class { +@@ -539,6 +540,9 @@ var FileDiff = class { + this.hunksRenderer.expandHunk(hunkIndex, direction, expansionLineCountOverride); + this.rerender(); + }; ++ getExpandedHunks() { ++ return this.hunksRenderer.getExpandedHunksMap(); ++ } + loadFilesIfNecessary() { + const fileDiff = this.getInputDiff(); + const { loadDiffFiles } = this.options; +@@ -724,7 +728,7 @@ var FileDiff = class { if (expandUnchanged) this.loadFilesIfNecessary(); this.hunksRenderer.setOptions(this.getHunksRendererOptions(this.options)); this.syncInteractionOptions(); @@ -716,7 +744,7 @@ index 12f4f8949f..e33a5ed319 100644 const { disableErrorHandling = false, disableFileHeader = false } = this.options; if (disableFileHeader) { if (this.headerElement != null) { -@@ -740,11 +741,22 @@ var FileDiff = class { +@@ -740,11 +744,22 @@ var FileDiff = class { this.removeRenderedCode(); this.clearAuxiliaryNodes(); try { @@ -744,7 +772,7 @@ index 12f4f8949f..e33a5ed319 100644 } catch (error) { if (disableErrorHandling) throw error; diff --git a/dist/components/VirtualizedFile.d.ts b/dist/components/VirtualizedFile.d.ts -index 90ef575932..0fd262cbb7 100644 +index 90ef575932174c3bc73e5051d2d75c3353631b75..0fd262cbb7e02aac1cf54954a9ef2438a86ba136 100644 --- a/dist/components/VirtualizedFile.d.ts +++ b/dist/components/VirtualizedFile.d.ts @@ -50,7 +50,7 @@ declare class VirtualizedFile extend @@ -757,7 +785,7 @@ index 90ef575932..0fd262cbb7 100644 protected finalizeRender(): void; private updatePendingRender; diff --git a/dist/components/VirtualizedFile.js b/dist/components/VirtualizedFile.js -index 95d4ca8fdf..24a980672d 100644 +index 95d4ca8fdf9e176e23d354094a11022eefdb2321..24a980672d06bfce2d49bf6938ec3782158c8d29 100644 --- a/dist/components/VirtualizedFile.js +++ b/dist/components/VirtualizedFile.js @@ -23,6 +23,7 @@ var VirtualizedFile = class extends File { @@ -823,7 +851,7 @@ index 95d4ca8fdf..24a980672d 100644 if (!this.isSimpleMode()) this.computeApproximateSize(true); else if (shouldUpdateBuffer && previousRenderRange != null && file != null) { diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/VirtualizedFileDiff.js -index eb34dd4a77..821bb0ab11 100644 +index eb34dd4a774cb06add3280fe6abd5f0dc292505e..821bb0ab114f52ec41eab54505a1eb08e511f47a 100644 --- a/dist/components/VirtualizedFileDiff.js +++ b/dist/components/VirtualizedFileDiff.js @@ -551,7 +551,7 @@ var VirtualizedFileDiff = class extends FileDiff { @@ -845,7 +873,7 @@ index eb34dd4a77..821bb0ab11 100644 workingDiff, diff: pendingRenderDiff diff --git a/dist/components/web-components.js b/dist/components/web-components.js -index 6e7b486a02..5b7290f181 100644 +index 6e7b486a02a0f9f9d2c57ea3d9e95b706925e72a..5b7290f1814f4883e9fd13ffdf22b9bacad461c0 100644 --- a/dist/components/web-components.js +++ b/dist/components/web-components.js @@ -2,8 +2,17 @@ import { DIFFS_TAG_NAME } from "../constants.js"; @@ -879,7 +907,7 @@ index 6e7b486a02..5b7290f181 100644 connectedCallback() { const shadowRoot = this.shadowRoot ?? this.attachShadow({ mode: "open" }); diff --git a/dist/editor/editor.js b/dist/editor/editor.js -index bf17cea32f..671e77b8cd 100644 +index bf17cea32f39ee60ff7520b17f44d99a8e7e98aa..671e77b8cd89a90d09376c0c93aeefb2e858eead 100644 --- a/dist/editor/editor.js +++ b/dist/editor/editor.js @@ -436,7 +436,9 @@ var Editor = class { @@ -907,7 +935,7 @@ index bf17cea32f..671e77b8cd 100644 if (newLineAnnotations != null) { this.#lineAnnotations = newLineAnnotations; diff --git a/dist/editor/searchPanel.d.ts b/dist/editor/searchPanel.d.ts -index 9542849786..1c1a51f56d 100644 +index 9542849786b98a7b11f98894847a2f53a9aac113..1c1a51f56da381105113d9b731e0573296edd9be 100644 --- a/dist/editor/searchPanel.d.ts +++ b/dist/editor/searchPanel.d.ts @@ -15,6 +15,8 @@ interface SearchPanelOptions { @@ -920,7 +948,7 @@ index 9542849786..1c1a51f56d 100644 scrollToMatch: (nextMatch: MatchRange, retainFocus: boolean) => void; applyReplace: (edits: ResolvedTextEdit[]) => void; diff --git a/dist/editor/searchPanel.js b/dist/editor/searchPanel.js -index df1ba637c2..676751bb0e 100644 +index df1ba637c2f7834f21f087eba7a416c802f4ac68..676751bb0ea518e24df95477183f647a78a59c98 100644 --- a/dist/editor/searchPanel.js +++ b/dist/editor/searchPanel.js @@ -12,7 +12,7 @@ var SearchPanelWidget = class { @@ -942,7 +970,7 @@ index df1ba637c2..676751bb0e 100644 this.#inputElement.select(); }; diff --git a/dist/managers/ResizeManager.d.ts b/dist/managers/ResizeManager.d.ts -index 3efafc4132..9e5b7511f9 100644 +index 3efafc41324e09c546e66645c0dc474e68417896..9e5b7511f9f82702c4e15fe7063b3e9a0b05a79d 100644 --- a/dist/managers/ResizeManager.d.ts +++ b/dist/managers/ResizeManager.d.ts @@ -5,6 +5,8 @@ interface ResizeManagerSetupOptions { @@ -955,7 +983,7 @@ index 3efafc4132..9e5b7511f9 100644 private static managersByElement; private static getResizeObserver; diff --git a/dist/managers/ResizeManager.js b/dist/managers/ResizeManager.js -index 9724cb1e34..14145d8329 100644 +index 9724cb1e344af1ac8763b592f671a9ceb63f1a76..14145d832900e7a9734786dbb6223d03424f7cb6 100644 --- a/dist/managers/ResizeManager.js +++ b/dist/managers/ResizeManager.js @@ -19,6 +19,7 @@ var ResizeManager = class ResizeManager { @@ -975,7 +1003,7 @@ index 9724cb1e34..14145d8329 100644 } applyAnnotationUpdates(annotationUpdates) { diff --git a/dist/react/CodeView.d.ts b/dist/react/CodeView.d.ts -index 12e12c50cd..684c7a89fa 100644 +index 12e12c50cd0442c230fb5a9ac00fb097f6dfb01e..684c7a89faa6a7ef0d81242159fec586ba1e51a9 100644 --- a/dist/react/CodeView.d.ts +++ b/dist/react/CodeView.d.ts @@ -57,6 +57,8 @@ interface CodeViewBaseProps { @@ -988,7 +1016,7 @@ index 12e12c50cd..684c7a89fa 100644 renderHeaderPrefix?(item: CodeViewItem): ReactNode; renderHeaderFilenameSuffix?(item: CodeViewItem): ReactNode; diff --git a/dist/react/CodeView.js b/dist/react/CodeView.js -index 4dfed6bd0a..ac35a5a455 100644 +index 4dfed6bd0af6511b7325620ae78d25a8857cfa8b..ac35a5a455bfd3228c8976e4d97eba8bc0e4d75c 100644 --- a/dist/react/CodeView.js +++ b/dist/react/CodeView.js @@ -24,7 +24,7 @@ function createDefaultCache(controlled) { @@ -1053,7 +1081,7 @@ index 4dfed6bd0a..ac35a5a455 100644 if (hasGutterRenderer) managedOptions.renderGutterUtility = noopRender; if (hasCodeViewHeader) managedOptions.renderCodeViewHeader = noopRender; diff --git a/dist/utils/createFileHeaderElement.js b/dist/utils/createFileHeaderElement.js -index 39fe30e47b..600442e3eb 100644 +index 39fe30e47b3e524a72eec9b25bdcd37559b7b9d3..600442e3eb9697e8657f5f7e93ff282309960d10 100644 --- a/dist/utils/createFileHeaderElement.js +++ b/dist/utils/createFileHeaderElement.js @@ -73,12 +73,12 @@ function createMetadataElement(fileDiff) { @@ -1072,10 +1100,10 @@ index 39fe30e47b..600442e3eb 100644 })); } diff --git a/package.json b/package.json -index 0cc43c4a7d..60b7d17d61 100644 +index 0cc43c4a7de60dc1a80a4b383f1fe4a477ae6d07..c07cab5ccd6c9c014aecb6de6aef3c102c3d3aa0 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" @@ -1091,6 +1119,10 @@ index 0cc43c4a7d..60b7d17d61 100644 + "./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 fa3d823395445..1299a6be36be5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -367,7 +367,7 @@ patchedDependencies: '@ff-labs/fff-node@0.11.0': cc71fcec015f4d39bc96ae2e2bf6b0ca678124c722c66a7841efce9d34b5983c '@legendapp/list@3.3.5': 6cbdacd8c380d9a5afb9f73d4198dc2cc44be6ef28a955b1183c781469307896 '@muse-code/sdk@1.4.6': 7b495b607fc5d76b444c7c4d58821e839726a30e6f3c53564f0d59f2d5b42101 - '@pierre/diffs@1.6.1': 322ac5f5db8b5715189c010802047f8235c3a45d8ed5ca0ba60fac266e2eba34 + '@pierre/diffs@1.6.1': 0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08 '@react-native-ai/apple@0.12.0': 2d09870c2848d185cb05b53ed823a46e12dba519324d8dd8e584e28731990f9d '@react-native-menu/menu@2.0.0': 910ec1bf22aa85a96fcf12299cd823bc30f702dd9b2d77b564d10b9ed709f7f6 '@react-navigation/native-stack@7.21.0': 990d575444c4af2ce8218b3b7a8280fe244c03abbdb32b822a2e862f791dbf42 @@ -557,7 +557,7 @@ importers: version: 1.9.1 '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=322ac5f5db8b5715189c010802047f8235c3a45d8ed5ca0ba60fac266e2eba34)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@pierre/highlights': specifier: 'catalog:' version: 0.2.0 @@ -1063,7 +1063,7 @@ importers: version: 1.8.0 '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=322ac5f5db8b5715189c010802047f8235c3a45d8ed5ca0ba60fac266e2eba34)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@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) @@ -1485,7 +1485,7 @@ importers: dependencies: '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=322ac5f5db8b5715189c010802047f8235c3a45d8ed5ca0ba60fac266e2eba34)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@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 @@ -15575,7 +15575,7 @@ snapshots: tslib: 2.8.1 webcrypto-core: 1.9.2 - '@pierre/diffs@1.6.1(patch_hash=322ac5f5db8b5715189c010802047f8235c3a45d8ed5ca0ba60fac266e2eba34)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': + '@pierre/diffs@1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': dependencies: '@pierre/highlights': 0.2.0 '@pierre/theme': 2.0.0 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index be4d3c29ea763..06552a84c32bf 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -372,8 +372,9 @@ patchedDependencies: expo-widgets@58.0.20: patches/expo-widgets@58.0.20.patch "@ff-labs/fff-node@0.11.0": patches/@ff-labs__fff-node@0.11.0.patch "@legendapp/list@3.3.5": patches/@legendapp__list@3.3.5.patch - # Exports the dependency-light subpaths mobile and the web diff parser import, and keeps - # measured wrapped-row heights above an edit (fileEditorVirtualization.test.ts). + # Exports the dependency-light subpaths mobile and the web diff parser import, exposes a + # file's expanded context for diff keyboard review, and keeps measured wrapped-row heights + # above an edit (fileEditorVirtualization.test.ts). "@pierre/diffs@1.6.1": patches/@pierre%2Fdiffs@1.6.1.patch "@react-native-ai/apple@0.12.0": patches/@react-native-ai__apple@0.12.0.patch "@react-native-menu/menu@2.0.0": patches/@react-native-menu__menu@2.0.0.patch From c660269a8d008c1a7d1911db84aaa768a44d5145 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:22:00 -0700 Subject: [PATCH 02/22] feat: add diff review keybinding commands and diffFocus context Adds the diff.* review commands with default single-key bindings, scoped by the diffFocus and diffLineMode when-variables, and reports diffFocus from the shortcut dispatcher. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/keybindings.test.ts | 30 +++++++++++++++++++++++++++ apps/web/src/lib/diffFocus.ts | 7 +++++++ apps/web/src/shortcuts/dispatcher.ts | 2 ++ packages/contracts/src/keybindings.ts | 16 ++++++++++++++ packages/shared/src/keybindings.ts | 16 ++++++++++++++ 5 files changed, 71 insertions(+) create mode 100644 apps/web/src/lib/diffFocus.ts diff --git a/apps/web/src/keybindings.test.ts b/apps/web/src/keybindings.test.ts index 1f0a096fccaec..3fc981fc453b0 100644 --- a/apps/web/src/keybindings.test.ts +++ b/apps/web/src/keybindings.test.ts @@ -920,3 +920,33 @@ 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: " ", code: "Space" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], + [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..8c2feeb5a2aad --- /dev/null +++ b/apps/web/src/lib/diffFocus.ts @@ -0,0 +1,7 @@ +import { isEditableFocused } from "./editableFocus"; + +export function isDiffFocused(): boolean { + const activeElement = document.activeElement; + if (!(activeElement instanceof HTMLElement) || !activeElement.isConnected) return false; + return activeElement.closest("[data-diff-panel]") !== null && !isEditableFocused(); +} diff --git a/apps/web/src/shortcuts/dispatcher.ts b/apps/web/src/shortcuts/dispatcher.ts index 5ce799cf482d5..f753431c39e87 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/packages/contracts/src/keybindings.ts b/packages/contracts/src/keybindings.ts index 714e0a1d486a1..121054f6a478f 100644 --- a/packages/contracts/src/keybindings.ts +++ b/packages/contracts/src/keybindings.ts @@ -73,6 +73,22 @@ export const STATIC_KEYBINDING_COMMANDS = [ "view.reopenClosed", "pullRequest.copyNumber", "diff.toggle", + "diff.nextFile", + "diff.previousFile", + "diff.nextUnviewedFile", + "diff.previousUnviewedFile", + "diff.toggleFileCollapsed", + "diff.toggleFileViewed", + "diff.expandFile", + "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/shared/src/keybindings.ts b/packages/shared/src/keybindings.ts index 32292c0f9ef21..39e66988483e5 100644 --- a/packages/shared/src/keybindings.ts +++ b/packages/shared/src/keybindings.ts @@ -164,6 +164,22 @@ 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: "o", command: "diff.toggleFileCollapsed", when: "diffFocus && !diffLineMode" }, + { key: "v", command: "diff.toggleFileViewed", when: "diffFocus" }, + { key: "x", command: "diff.expandFile", when: "diffFocus && !diffLineMode" }, + { 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: "space", 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" }, From 66a487ef2fc760f38cf62b73efac5a825c61cfd2 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:24:27 -0700 Subject: [PATCH 03/22] feat: add diff auto-advance after marking viewed setting Co-Authored-By: Claude Opus 5.5 --- .../settings/DesktopClientSettings.test.ts | 1 + .../components/settings/SettingsPanels.tsx | 30 +++++++++++++++++++ .../src/components/settings/settingsSearch.ts | 6 ++++ packages/contracts/src/settings.ts | 2 ++ 4 files changed, 39 insertions(+) diff --git a/apps/desktop/src/settings/DesktopClientSettings.test.ts b/apps/desktop/src/settings/DesktopClientSettings.test.ts index ec5062f9105bf..9392205a66093 100644 --- a/apps/desktop/src/settings/DesktopClientSettings.test.ts +++ b/apps/desktop/src/settings/DesktopClientSettings.test.ts @@ -34,6 +34,7 @@ const clientSettings: ClientSettings = { confirmThreadDelete: false, confirmThreadUnpin: false, contextWindowMeterEnabled: false, + diffAutoAdvanceOnViewed: true, diffFilesCollapsed: true, diffIgnoreWhitespace: true, diffLayout: "stacked", diff --git a/apps/web/src/components/settings/SettingsPanels.tsx b/apps/web/src/components/settings/SettingsPanels.tsx index 362210f96eee1..3ac2c6ac188eb 100644 --- a/apps/web/src/components/settings/SettingsPanels.tsx +++ b/apps/web/src/components/settings/SettingsPanels.tsx @@ -515,6 +515,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"] : []), @@ -637,6 +640,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, @@ -1394,6 +1398,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 d28e467b0cf70..e29b66d0edd7a 100644 --- a/apps/web/src/components/settings/settingsSearch.ts +++ b/apps/web/src/components/settings/settingsSearch.ts @@ -564,6 +564,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/packages/contracts/src/settings.ts b/packages/contracts/src/settings.ts index e90b977fae32d..4520b59316ef6 100644 --- a/packages/contracts/src/settings.ts +++ b/packages/contracts/src/settings.ts @@ -417,6 +417,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))), @@ -1764,6 +1765,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), From 08ffbdb5be3f6a5c775dd2413ef67cea11c54247 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:39:27 -0700 Subject: [PATCH 04/22] feat(web): model diff review cursors and per-device viewed files Adds the pure row, gap, and file navigation logic behind keyboard review, and a persisted store of viewed files keyed by thread, path, and content fingerprint. Co-Authored-By: Claude Opus 5.5 --- .../components/diffs/diffReviewCursor.test.ts | 197 ++++++++++++++++ .../src/components/diffs/diffReviewCursor.ts | 210 ++++++++++++++++++ apps/web/src/diffViewedStore.test.ts | 72 ++++++ apps/web/src/diffViewedStore.ts | 64 ++++++ 4 files changed, 543 insertions(+) create mode 100644 apps/web/src/components/diffs/diffReviewCursor.test.ts create mode 100644 apps/web/src/components/diffs/diffReviewCursor.ts create mode 100644 apps/web/src/diffViewedStore.test.ts create mode 100644 apps/web/src/diffViewedStore.ts 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..5be7f53aaf314 --- /dev/null +++ b/apps/web/src/components/diffs/diffReviewCursor.test.ts @@ -0,0 +1,197 @@ +import { parseDiffFromFile } from "@pierre/diffs"; +import { describe, expect, it } from "vite-plus/test"; + +import { + adjacentFileKey, + adjacentUnviewedFileKey, + cursorFromSelectedRange, + cursorSelectedRange, + type DiffRow, + nearestGapExpansion, + stepRow, + topToBottomRange, + visibleDiffRows, + wholeFileExpansions, +} 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( + wholeFileExpansions(fileDiff, unified, NO_EXPANSIONS).map((expansion) => [ + expansion.hunkIndex, + { fromStart: expansion.lineCount, 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("wholeFileExpansions", () => { + it("expands every gap while lines are hidden, then folds back what was expanded", () => { + expect(wholeFileExpansions(fileDiff, unified, NO_EXPANSIONS).map((e) => e.hunkIndex)).toEqual([ + 0, 1, 2, + ]); + const expanded = new Map([ + [0, { fromStart: 1_000_000, fromEnd: 0 }], + [1, { fromStart: 1_000_000, fromEnd: 3 }], + [2, { fromStart: 1_000_000, fromEnd: 0 }], + ]); + const rows = visibleDiffRows(fileDiff, "unified", expanded); + expect(wholeFileExpansions(fileDiff, rows, expanded)).toEqual([ + { hunkIndex: 0, direction: "up", lineCount: -1_000_000 }, + { hunkIndex: 1, direction: "up", lineCount: -1_000_000 }, + { hunkIndex: 1, direction: "down", lineCount: -3 }, + { hunkIndex: 2, direction: "up", lineCount: -1_000_000 }, + ]); + }); +}); + +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(); + }); +}); diff --git a/apps/web/src/components/diffs/diffReviewCursor.ts b/apps/web/src/components/diffs/diffReviewCursor.ts new file mode 100644 index 0000000000000..b8908d6f2fbce --- /dev/null +++ b/apps/web/src/components/diffs/diffReviewCursor.ts @@ -0,0 +1,210 @@ +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; +} + +// Expansion regions are clamped to their gap when read, so this reveals any gap in full, +// including the trailing gap of a partial diff whose size is unknown until it loads. +const WHOLE_GAP_LINE_COUNT = 1_000_000; + +export interface DiffGapExpansionCount extends DiffGapExpansion { + readonly lineCount: number; +} + +/** + * Shows the whole file while anything is hidden or nothing was expanded yet; otherwise folds back + * exactly what the viewer reports as expanded. + */ +export function wholeFileExpansions( + fileDiff: FileDiffMetadata, + rows: ReadonlyArray, + expandedHunks: ReadonlyMap, +): ReadonlyArray { + const hasHiddenLines = rows.some((row) => row.hiddenBefore > 0 || row.hiddenAfter > 0); + const folds = [...expandedHunks].flatMap(([hunkIndex, region]) => [ + ...(region.fromStart > 0 + ? [{ hunkIndex, direction: "up" as const, lineCount: -region.fromStart }] + : []), + ...(region.fromEnd > 0 + ? [{ hunkIndex, direction: "down" as const, lineCount: -region.fromEnd }] + : []), + ]); + if (!hasHiddenLines && folds.length > 0) return folds; + return Array.from({ length: fileDiff.hunks.length + 1 }, (_, hunkIndex) => ({ + hunkIndex, + direction: "up" as const, + lineCount: WHOLE_GAP_LINE_COUNT, + })); +} + +/** 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 }; +} + +/** 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/diffViewedStore.test.ts b/apps/web/src/diffViewedStore.test.ts new file mode 100644 index 0000000000000..8f95ba70cc54f --- /dev/null +++ b/apps/web/src/diffViewedStore.test.ts @@ -0,0 +1,72 @@ +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}`, + "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}`, "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 }), + }, + ), +); From ab1774f6874e923917e5cd669f03fa76d3a961ff Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:39:48 -0700 Subject: [PATCH 05/22] feat(web): review the Diff panel from the keyboard The Diff panel gets file, line, and selection modes driven by the diff.* commands, a Viewed checkbox and counter, and a one-file-at-a-time toggle. Comment drafts return focus to the panel when they close. Co-Authored-By: Claude Opus 5.5 --- .../src/components/DiffPanel.browser.test.tsx | 2 + apps/web/src/components/DiffPanel.tsx | 294 ++++++++++---- apps/web/src/components/DiffPanelShell.tsx | 14 +- .../AnnotatableCodeView.browser.test.tsx | 44 ++- .../components/diffs/AnnotatableCodeView.tsx | 67 ++-- .../components/diffs/useDiffReviewKeyboard.ts | 369 ++++++++++++++++++ 6 files changed, 677 insertions(+), 113 deletions(-) create mode 100644 apps/web/src/components/diffs/useDiffReviewKeyboard.ts diff --git a/apps/web/src/components/DiffPanel.browser.test.tsx b/apps/web/src/components/DiffPanel.browser.test.tsx index 505c11130d692..3a2fbaa6a86c0 100644 --- a/apps/web/src/components/DiffPanel.browser.test.tsx +++ b/apps/web/src/components/DiffPanel.browser.test.tsx @@ -36,6 +36,8 @@ const state = vi.hoisted(() => ({ vi.mock("~/state/server", () => ({ serverEnvironment: { configValueAtom: () => state.config }, + // Read by the shortcut dispatcher only when a key is pressed. + primaryServerKeybindingsAtom: null, })); vi.mock("~/state/review", () => ({ reviewEnvironment: { diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 0f1d1e9e28379..10c65161efa82 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -6,12 +6,14 @@ import { squashAtomCommandFailure, } from "@t3tools/client-runtime/state/runtime"; import { safeErrorLogAttributes } from "@t3tools/client-runtime/errors"; +import { scopedThreadKey } from "@t3tools/client-runtime/environment"; import type { ReviewBranchCommitsResult, ScopedThreadRef, RunId } from "@t3tools/contracts"; import { ArrowRightIcon, CheckIcon, ChevronDownIcon, Columns2Icon, + FileIcon, FlaskConicalOffIcon, FolderTreeIcon, PilcrowIcon, @@ -38,6 +40,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"; @@ -65,7 +68,8 @@ import { formatRelativeTimeLabel, formatShortTimestamp } from "../timestampForma import { DiffFilePathCopyButton } from "./DiffFilePathCopyButton"; import { DiffPanelLoadingState, DiffPanelShell, type DiffPanelMode } from "./DiffPanelShell"; import { DiffStatLabel } from "./chat/DiffStatLabel"; -import { AnnotatableCodeView } from "./diffs/AnnotatableCodeView"; +import { AnnotatableCodeView, type AnnotatableCodeViewHandle } from "./diffs/AnnotatableCodeView"; +import { type DiffReviewFile, useDiffReviewKeyboard } from "./diffs/useDiffReviewKeyboard"; import { DiffFileTree } from "./diffs/DiffFileTree"; import { FileExplorerPane } from "./files/FileExplorerPane"; import { diffFileTreeEntries } from "./diffs/diffFileTree.logic"; @@ -73,6 +77,7 @@ import { Button } from "./ui/button"; import { MorphIcon } from "~/components/MorphIcon"; import { ToggleGroup, Toggle } from "./ui/toggle-group"; import { Switch } from "./ui/switch"; +import { Checkbox } from "./ui/checkbox"; import { Combobox, ComboboxEmpty, @@ -112,9 +117,10 @@ import { useOpenLink } from "../browser/useOpenLink"; type DiffThemeType = "light" | "dark"; const AUTOMATIC_BASE_REF = "__automatic_base_ref__"; const DIFF_FILE_TREE_STORAGE_KEY = "t3code.diffFileTreeOpen"; +const DIFF_SINGLE_FILE_STORAGE_KEY = "t3code.diffSingleFile"; const fileEntryCache = new WeakMap< FileDiffMetadata, - { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number } + { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number; viewedFingerprint: string } >(); function getCachedFileEntry(fileDiff: FileDiffMetadata) { @@ -124,6 +130,7 @@ function getCachedFileEntry(fileDiff: FileDiffMetadata) { fileDiff, fileKey: buildFileDiffIdentityKey(fileDiff), fileVersion: buildFileDiffContentVersion(fileDiff), + viewedFingerprint: diffViewedFingerprint(fileDiff), }; fileEntryCache.set(fileDiff, entry); return entry; @@ -136,39 +143,71 @@ function DiffFileCollapseToggle({ fileKey, collapsed, unavailable, + hasCursor, onToggle, }: { filePath: string; fileKey: string; collapsed: boolean; unavailable: boolean; + hasCursor: boolean; onToggle: (fileKey: string) => void; }) { return ( - - { - event.stopPropagation(); - onToggle(fileKey); - }} - /> - } - > - + {hasCursor ? ( + // The sticky header is the nearest positioned box, so this outlines the whole header. + - - {collapsed ? "Expand diff" : "Collapse diff"} - + ) : null} + + { + event.stopPropagation(); + onToggle(fileKey); + }} + /> + } + > + + + {collapsed ? "Expand diff" : "Collapse diff"} + + + ); +} + +function DiffFileViewedToggle({ + filePath, + viewed, + onViewedChange, +}: { + filePath: string; + viewed: boolean; + onViewedChange: (viewed: boolean) => void; +}) { + return ( + ); } @@ -212,6 +251,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, @@ -226,6 +266,11 @@ export default function DiffPanel({ false, Schema.Boolean, ); + const [singleFile, setSingleFile] = useLocalStorage( + DIFF_SINGLE_FILE_STORAGE_KEY, + false, + Schema.Boolean, + ); const [hideTestFiles, setHideTestFiles] = useHideTestFiles(); const [baseRefQuery, setBaseRefQuery] = useState(""); // The open commit menu keeps the list it opened with, so rows do not move under the pointer. @@ -636,8 +681,84 @@ export default function DiffPanel({ .getState() .updateCollapsedFileKeys(collapseScopeKey, defaultCollapsedDiffFileKeys, update); }; + const isFileReady = (fileDiff: FileDiffMetadata) => + !lazySource || !readyFilePaths || readyFilePaths.has(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 setDiffFileCollapsed = (fileKey: string, collapsed: boolean) => + 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, + filePath: resolveFileDiffPath(fileDiff), + fileDiff, + rendered: isFileReady(fileDiff) && !isFilePending(fileDiff), + })); + const viewedFileCount = shownFileEntries.filter(({ fileDiff }) => isFileViewed(fileDiff)).length; + const codeViewReveal = useCodeViewFileReveal(); + const viewerHandleRef = useRef(null); + const attachViewer = useStableCallback((handle: AnnotatableCodeViewHandle | null) => { + viewerHandleRef.current = handle; + codeViewReveal.attach(handle); + }); + const revealFileAt = (index: number) => { + const file = renderableFileEntries[index]; + if (!file) return; + if (lazySource && index >= settledFileCount) { + requestFile(index); + } + codeViewReveal.request(file.fileKey); + }; + const { + rootRef: reviewRootRef, + reviewRef, + ...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), + setCollapsed: setDiffFileCollapsed, + isViewed: (file) => isFileViewed(file.fileDiff), + setViewed: (file, viewed) => setFileViewed(file.fileDiff, viewed), + revealFile: (file) => + revealFileAt(renderableFileEntries.findIndex(({ fileKey }) => fileKey === file.fileKey)), + singleFile, + setSingleFile, + }); const renderLoadingBoundary = () => - settledFileCount < renderableFiles.length ? ( + review.singleFileKey === undefined && settledFileCount < renderableFiles.length ? ( - !lazySource || !readyFilePaths || readyFilePaths.has(resolveFileDiffPath(fileDiff)), + ({ fileDiff, fileKey }) => + isFileReady(fileDiff) && + (review.singleFileKey === undefined || fileKey === review.singleFileKey), ) .map(({ fileDiff, fileKey, fileVersion }) => ({ fileDiff, @@ -654,30 +776,22 @@ export default function DiffPanel({ fileKey, fileVersion, collapsed: - collapsedDiffFileKeys.has(fileKey) || fileDiff.cacheKey?.endsWith(":pending") === true, + isFilePending(fileDiff) || + (fileKey !== review.singleFileKey && collapsedDiffFileKeys.has(fileKey)), })); - const pendingFiles = renderableFiles.filter( - (file) => file.cacheKey?.endsWith(":pending") && isFileShown(file), - ); + 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 codeViewReveal = useCodeViewFileReveal(); const revealDiffFile = (filePath: string) => { const index = renderableFileEntries.findIndex( (candidate) => resolveFileDiffPath(candidate.fileDiff) === filePath, ); const file = renderableFileEntries[index]; if (!file) return false; - updateCollapsedDiffFiles((current) => { - const next = new Set(current); - next.delete(file.fileKey); - return next; - }); - if (lazySource && index >= settledFileCount) { - requestFile(index); - } - codeViewReveal.request(file.fileKey); + setDiffFileCollapsed(file.fileKey, false); + review.selectFile(filePath); + revealFileAt(index); return true; }; @@ -1090,6 +1204,11 @@ export default function DiffPanel({ headerCommitSha !== null && "gap-0", )} > + {shownFileEntries.length > 0 ? ( + + {viewedFileCount} / {shownFileEntries.length} viewed + + ) : null} {codeViewFiles.length > 0 || (isGitSelection && selectedGitSource?.files?.length) ? ( + {diffFileKeys.length > 0 && ( + + + } + > + + + + {singleFile ? "Show all files" : "Show one file at a time"} + + + )} + {!activeThread ? (
Select a thread to inspect turn diffs. @@ -1341,8 +1480,12 @@ export default function DiffPanel({ 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) { + // Header controls own their clicks; bubbling a chevron click would toggle twice. + if ( + node instanceof HTMLButtonElement || + node instanceof HTMLAnchorElement || + node instanceof HTMLLabelElement + ) { return; } } @@ -1388,7 +1531,11 @@ export default function DiffPanel({ > { - const stat = fileStats.get(resolveFileDiffPath(fileDiff)); - return stat ? ( - - ) : null; - }, } : {})} - onRevealSearchMatch={unfoldDiffFile} - renderHeaderPrefix={(fileDiff, fileKey) => { - const unavailable = fileDiff.cacheKey?.endsWith(":pending") === true; + renderHeaderMetadata={(fileDiff) => { + const path = resolveFileDiffPath(fileDiff); + const stat = lazySource ? fileStats.get(path) : undefined; return ( - + <> + {stat ? ( + + ) : null} + {isFilePending(fileDiff) ? null : ( + { + setFileViewed(fileDiff, viewed); + setDiffFileCollapsed(getCachedFileEntry(fileDiff).fileKey, viewed); + }} + /> + )} + ); }} + onRevealSearchMatch={unfoldDiffFile} + renderHeaderPrefix={(fileDiff, fileKey, collapsed) => ( + + )} options={{ diffStyle: diffLayout === "split" ? "split" : "unified", lineDiffType: "none", @@ -1446,7 +1603,10 @@ export default function DiffPanel({ preferredHighlighter: PREFERRED_HIGHLIGHTER, themeType: resolvedTheme as DiffThemeType, stickyHeaders: true, - onPostRender: codeViewReveal.flush, + onPostRender: () => { + codeViewReveal.flush(); + review.flushScroll(); + }, ...(currentLoadDiffFiles ? { loadDiffFiles } : {}), }} /> @@ -1461,7 +1621,7 @@ export default function DiffPanel({ 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.
[]; + codeViewKey: string; + pendingFiles: FileDiffMetadata[]; +}) { + const [selectedLines, setSelectedLines] = useState(null); + return ( + null} + renderHeaderPrefix={() => null} + onRevealSearchMatch={() => {}} + /> + ); +} + function view( files: ReturnType[], codeViewKey = "initial", @@ -82,18 +109,7 @@ function view( ) { return ( - null} - renderHeaderPrefix={() => null} - onRevealSearchMatch={() => {}} - /> + ); } diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 81bcba75aa797..9de690b257e7e 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -2,13 +2,14 @@ import type { AnnotationSide, CodeViewDiffItem, CodeViewItem, + CodeViewLineSelection, DiffLineAnnotation, FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; import type { CodeViewHandle } from "@pierre/diffs/react"; import type { ScopedThreadRef } from "@t3tools/contracts"; -import { useState, type ReactNode, type Ref } from "react"; +import { useImperativeHandle, useState, type ReactNode, type Ref } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; import { fnv1a32, resolveFileDiffPath, resolveFileDiffPreviousPath } from "~/lib/diffRendering"; @@ -36,6 +37,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 { @@ -86,6 +91,10 @@ interface AnnotatableCodeViewProps { composerDraftTarget: ScopedThreadRef | DraftId; options: StyledDiffCodeViewOptions; viewerRef?: Ref; + reviewRef?: Ref; + selectedLines: CodeViewLineSelection | null; + onSelectedLinesChange: (selection: CodeViewLineSelection | null) => void; + onDraftClose?: () => void; className?: string; renderCodeViewFooter?: () => ReactNode; unsafeCSSExtra?: string; @@ -99,10 +108,6 @@ interface AnnotatableCodeViewProps { onRevealSearchMatch: (fileKey: string) => void; } -interface DiffSelectionContext { - item: CodeViewItem; -} - export function AnnotatableCodeView({ codeViewKey, files, @@ -112,6 +117,10 @@ export function AnnotatableCodeView({ composerDraftTarget, options, viewerRef, + reviewRef, + selectedLines, + onSelectedLinesChange, + onDraftClose, className, renderCodeViewFooter, unsafeCSSExtra, @@ -125,10 +134,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; @@ -150,7 +155,6 @@ export function AnnotatableCodeView({ ) { setDraft(null); setDraftText(""); - setSelectedLines(null); } const items: CodeViewDiffItem[] = files.map( ({ fileDiff, filePath, fileKey, fileVersion, collapsed }) => { @@ -192,12 +196,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); } }; @@ -216,32 +225,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, @@ -250,7 +255,9 @@ export function AnnotatableCodeView({ }, }, }); + return true; }; + useImperativeHandle(reviewRef, () => ({ beginComment })); return ( @@ -267,13 +274,15 @@ export function AnnotatableCodeView({ {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} items={items} selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} + onSelectedLinesChange={onSelectedLinesChange} onRevealSearchMatch={(item) => onRevealSearchMatch(item.id)} options={{ ...options, enableGutterUtility: draft === null, enableLineSelection: draft === null, - onGutterUtilityClick: beginComment, + onGutterUtilityClick: (range, { item }) => { + if (range) beginComment(item.id, range); + }, }} renderHeaderFilenameSuffix={(item) => item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts new file mode 100644 index 0000000000000..fd17f151c7eab --- /dev/null +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -0,0 +1,369 @@ +import type { + CodeViewLineScrollTarget, + CodeViewLineSelection, + FileDiffMetadata, + HunkExpansionRegion, +} from "@pierre/diffs"; +import type { KeybindingCommand } from "@t3tools/contracts"; +import { useEffect, useRef, useState } from "react"; + +import { useStableCallback } from "~/hooks/useStableCallback"; +import { type CommandHandlers, registerCommands } from "~/shortcuts/dispatcher"; + +import type { AnnotatableCodeViewHandle, AnnotatableCodeViewReview } from "./AnnotatableCodeView"; +import { + adjacentFileKey, + adjacentUnviewedFileKey, + cursorFromSelectedRange, + cursorSelectedRange, + type DiffReviewCursor, + type DiffRowRef, + nearestGapExpansion, + stepRow, + topToBottomRange, + visibleDiffRows, + wholeFileExpansions, +} from "./diffReviewCursor"; + +const DIFF_REVIEW_COMMANDS = [ + "diff.nextFile", + "diff.previousFile", + "diff.nextUnviewedFile", + "diff.previousUnviewedFile", + "diff.toggleFileCollapsed", + "diff.toggleFileViewed", + "diff.expandFile", + "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(); + +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: () => AnnotatableCodeViewHandle | null; + readonly isCollapsed: (fileKey: string) => boolean; + readonly setCollapsed: (fileKey: string, collapsed: boolean) => void; + readonly isViewed: (file: DiffReviewFile) => boolean; + readonly setViewed: (file: DiffReviewFile, viewed: boolean) => void; + /** Loads the file if needed and scrolls its header into view. */ + readonly revealFile: (file: DiffReviewFile) => void; + /** Shows only the cursor's file, forced open. */ + readonly singleFile: boolean; + readonly setSingleFile: (singleFile: boolean) => 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 inside the panel. + */ +export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { + const { scopeKey, files } = input; + const [cursorState, setCursorState] = useState<{ + readonly scopeKey: string | null; + readonly cursor: DiffReviewCursor | null; + }>({ scopeKey, cursor: null }); + const rootRef = useRef(null); + const reviewRef = useRef(null); + const pendingScrollRef = useRef(null); + + const storedCursor = cursorState.scopeKey === scopeKey ? cursorState.cursor : null; + const cursorFile = storedCursor + ? (files.find((file) => file.fileKey === storedCursor.fileKey) ?? + files.find((file) => file.filePath === storedCursor.filePath) ?? + null) + : null; + const singleFileKey = input.singleFile ? (cursorFile ?? files[0])?.fileKey : undefined; + const isCollapsed = (fileKey: string) => fileKey !== singleFileKey && input.isCollapsed(fileKey); + const cursor: DiffReviewCursor | null = + storedCursor === null || cursorFile === null + ? null + : storedCursor.mode !== "file" && (!cursorFile.rendered || isCollapsed(cursorFile.fileKey)) + ? { mode: "file", fileKey: cursorFile.fileKey, filePath: cursorFile.filePath } + : { ...storedCursor, fileKey: cursorFile.fileKey }; + const range = cursor ? cursorSelectedRange(cursor) : null; + const selectedLines: CodeViewLineSelection | null = + cursor && range ? { id: cursor.fileKey, range } : null; + + const setCursor = (next: DiffReviewCursor | null) => setCursorState({ scopeKey, cursor: 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) => + visibleDiffRows( + file.fileDiff, + input.diffStyle, + fileDiffInstance(file.fileKey)?.getExpandedHunks() ?? 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 scrollToRow = (file: DiffReviewFile, row: DiffRowRef) => { + pendingScrollRef.current = { + type: "line", + id: file.fileKey, + lineNumber: row.lineNumber, + side: row.side, + align: "nearest", + }; + flushScroll(); + }; + const moveToFile = (fileKey: string | null) => { + const file = files.find((candidate) => candidate.fileKey === fileKey); + if (!file) return; + pendingScrollRef.current = null; + setCursor(fileCursor(file)); + input.revealFile(file); + }; + + const runCommand = (command: DiffReviewCommand): boolean => { + 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; + moveToFile(adjacentFileKey(fileKeys, currentKey, command === "diff.nextFile" ? 1 : -1)); + return true; + } + case "diff.nextUnviewedFile": + case "diff.previousUnviewedFile": { + if (files.length === 0) return false; + const delta = command === "diff.nextUnviewedFile" ? 1 : -1; + moveToFile(adjacentUnviewedFileKey(fileKeys, currentKey, delta, isKeyViewed)); + return true; + } + case "diff.toggleFileCollapsed": { + if (!cursorFile) return false; + input.setCollapsed(cursorFile.fileKey, !isCollapsed(cursorFile.fileKey)); + return true; + } + case "diff.toggleFileViewed": { + if (!cursorFile) 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.expandFile": { + if (!cursorFile) return false; + if (!cursorFile.rendered || !canExpand(cursorFile)) return true; + input.setCollapsed(cursorFile.fileKey, false); + const instance = fileDiffInstance(cursorFile.fileKey); + if (!instance) return true; + const expansions = wholeFileExpansions( + cursorFile.fileDiff, + rowsFor(cursorFile), + instance.getExpandedHunks(), + ); + for (const { hunkIndex, direction, lineCount } of expansions) { + instance.expandHunk(hunkIndex, direction, lineCount); + } + return true; + } + case "diff.toggleSingleFile": { + const file = cursorFile ?? files[0]; + if (!file) return false; + setCursor(fileCursor(file)); + input.setSingleFile(!input.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); + 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 next = stepRow(rows, cursor.mode === "line" ? cursor.row : cursor.head, delta); + if (!next) 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 gap = nearestGapExpansion( + cursorFile.fileDiff, + rowsFor(cursorFile), + cursor.mode === "line" ? cursor.row : cursor.head, + command === "diff.expandBelow" ? "below" : "above", + ); + if (gap) instance.expandHunk(gap.hunkIndex, gap.direction); + return true; + } + case "diff.comment": { + if (!cursor || !cursorFile || !range) return false; + reviewRef.current?.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 } + : { mode: "file", fileKey: cursor.fileKey, filePath: cursor.filePath }, + ); + 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(); + } + return handled; + }); + const isLineMode = useStableCallback(() => 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); + } + // Capture phase, so the keys reach the panel before type-to-focus sends them to the composer. + return registerCommands(handlers, { + capture: true, + when: () => rootRef.current?.contains(document.activeElement) === true, + context: () => ({ diffLineMode: isLineMode() }), + }); + }, [handleCommand, isLineMode]); + + return { + cursor, + singleFileKey, + selectedLines, + rootRef, + reviewRef, + flushScroll, + /** Puts the file cursor on a file the user picked another way, such as the file tree. */ + toggleSingleFile: () => runCommand("diff.toggleSingleFile"), + selectFile: (filePath: string) => { + const file = files.find((candidate) => candidate.filePath === filePath); + if (file) setCursor(fileCursor(file)); + }, + onSelectedLinesChange: (selection: CodeViewLineSelection | null) => { + if (selection === null) { + 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: () => { + if (cursor?.mode === "select") { + setCursor({ + mode: "line", + fileKey: cursor.fileKey, + filePath: cursor.filePath, + row: cursor.head, + }); + } + rootRef.current?.focus({ preventScroll: true }); + }, + }; +} From 164b87ca8b14ef6ac97206d0a86f3cd8791dde16 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:40:22 -0700 Subject: [PATCH 06/22] docs: describe keyboard review in the Diff panel Co-Authored-By: Claude Opus 5.5 --- docs/user/keybindings.md | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index dd256211a8516..b484a347839d2 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -64,6 +64,38 @@ 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 to review it without the mouse. The panel starts 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. +- `v` marks the file viewed, which also folds it. Press it again to unmark. +- `o` folds or unfolds the file. +- `x` shows the whole file. Press it again to hide the unchanged lines. +- `f` shows only the current file. `j`, `k`, and `n` switch which file it shows. + +On lines: + +- `j` and `k` move one line. +- `Shift+j` and `Shift+k` show more of the unchanged lines below or above. +- `Space` comments on the line. After you send or cancel the comment, you are + back on the line. +- `Shift+v` starts a selection. `j` and `k` extend it, and `Space` comments on + the selected lines. +- `v` marks the file viewed and returns to files. + +Viewed marks belong to this device and thread. A file that changes after you +view it shows as not viewed again. To jump to the next unviewed file after each +mark, turn on **Settings → Diffs → Auto-advance after marking 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 @@ -113,8 +145,10 @@ Join modifiers and a key with `+`, such as `mod+shift+d` or `ctrl+l`. ## When conditions Available context keys are `terminalFocus`, `terminalOpen`, `previewFocus`, -`previewOpen`, `modelPickerOpen`, `usagePageOpen`, `composerFocus`, `draftThreadRoute`, -`turnRunning`, `editableFocus`, `isWeb`, and `isDesktop`. +`previewOpen`, `diffFocus`, `diffLineMode`, `modelPickerOpen`, `usagePageOpen`, +`composerFocus`, `draftThreadRoute`, `turnRunning`, `editableFocus`, `isWeb`, and +`isDesktop`. `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 From 7d9957143f64e4907de3dac05395d4b1e673e100 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:40:34 -0700 Subject: [PATCH 07/22] feat(web): label the whole-file diff command by what it does Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/settings/KeybindingsSettings.logic.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/web/src/components/settings/KeybindingsSettings.logic.ts b/apps/web/src/components/settings/KeybindingsSettings.logic.ts index fe9b47b4dcadc..4eace440a51bd 100644 --- a/apps/web/src/components/settings/KeybindingsSettings.logic.ts +++ b/apps/web/src/components/settings/KeybindingsSettings.logic.ts @@ -382,6 +382,7 @@ export function commandLabel(command: KeybindingCommand): string { if (usagePeriod) return `Usage: Period: ${usagePeriod.label}`; if (command === "view.reopenClosed") return "Reopen Closed Tab"; if (command === "preview.focusUrl") return "Preview: Focus URL"; + if (command === "diff.expandFile") return "Diff: Show Whole File"; const raw = String(command); if (raw.startsWith("script.") && raw.endsWith(".run")) { return `Run Script: ${titleCaseCommandSegment(raw.slice("script.".length, -".run".length))}`; From 4503fe107f1450016256866f598625beaaebeb8f Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:57:50 -0700 Subject: [PATCH 08/22] refactor(web): tidy diff review keyboard hook Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/diffs/useDiffReviewKeyboard.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index fd17f151c7eab..ee25cc2718224 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -300,7 +300,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { setCursor( cursor.mode === "select" ? { mode: "line", fileKey: cursor.fileKey, filePath: cursor.filePath, row: cursor.head } - : { mode: "file", fileKey: cursor.fileKey, filePath: cursor.filePath }, + : fileCursor(cursor), ); return true; } @@ -339,8 +339,8 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { rootRef, reviewRef, flushScroll, - /** Puts the file cursor on a file the user picked another way, such as the file tree. */ toggleSingleFile: () => runCommand("diff.toggleSingleFile"), + /** 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)); From 8e1310b99d6b35f5c04abb8a86d27450725df03e Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 15:40:56 -0700 Subject: [PATCH 09/22] feat(web): fold diff files with Enter or Space and show review shortcuts Enter and Space replace `o` for folding a file, and `x` (show the whole file) is gone. A control reached with Tab still gets Enter and Space. The Viewed checkbox and the gutter "+" now show tooltips with their current shortcut. Co-Authored-By: Claude Opus 5.5 --- .../src/components/DiffPanel.browser.test.tsx | 12 +- apps/web/src/components/DiffPanel.tsx | 40 ++++-- .../components/diffs/AnnotatableCodeView.tsx | 131 +++++++++++------- .../components/diffs/diffReviewCursor.test.ts | 26 +--- .../src/components/diffs/diffReviewCursor.ts | 34 ----- .../components/diffs/useDiffReviewKeyboard.ts | 35 +++-- .../settings/KeybindingsSettings.logic.ts | 1 - apps/web/src/keybindings.test.ts | 2 + docs/user/keybindings.md | 3 +- packages/contracts/src/keybindings.ts | 1 - packages/shared/src/keybindings.ts | 4 +- 11 files changed, 138 insertions(+), 151 deletions(-) diff --git a/apps/web/src/components/DiffPanel.browser.test.tsx b/apps/web/src/components/DiffPanel.browser.test.tsx index 3a2fbaa6a86c0..24c5cba3ca1df 100644 --- a/apps/web/src/components/DiffPanel.browser.test.tsx +++ b/apps/web/src/components/DiffPanel.browser.test.tsx @@ -34,11 +34,13 @@ const state = vi.hoisted(() => ({ commits: null as unknown as ResultAtom, })); -vi.mock("~/state/server", () => ({ - serverEnvironment: { configValueAtom: () => state.config }, - // Read by the shortcut dispatcher only when a key is pressed. - primaryServerKeybindingsAtom: null, -})); +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 10c65161efa82..b052ddee819ce 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -101,7 +101,8 @@ import { import { Tooltip, TooltipPopup, TooltipTrigger } from "./ui/tooltip"; import { useEnvironmentQuery } from "../state/query"; import { useAtomCommand } from "../state/use-atom-command"; -import { serverEnvironment } from "../state/server"; +import { primaryServerKeybindingsAtom, serverEnvironment } from "../state/server"; +import { shortcutLabelForCommand } from "../keybindings"; import { reviewEnvironment } from "../state/review"; import { vcsEnvironment } from "../state/vcs"; import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices"; @@ -193,21 +194,33 @@ function DiffFileCollapseToggle({ function DiffFileViewedToggle({ filePath, viewed, + shortcutLabel, onViewedChange, }: { filePath: string; viewed: boolean; + shortcutLabel: string | null; onViewedChange: (viewed: boolean) => void; }) { + const action = viewed ? "Mark as not viewed" : "Mark as viewed"; return ( - + + + } + > + onViewedChange(checked)} + /> + Viewed + + + {shortcutLabel ? `${action} (${shortcutLabel})` : action} + + ); } @@ -723,6 +736,13 @@ export default function DiffPanel({ rendered: isFileReady(fileDiff) && !isFilePending(fileDiff), })); const viewedFileCount = shownFileEntries.filter(({ fileDiff }) => isFileViewed(fileDiff)).length; + const keybindings = useAtomValue(primaryServerKeybindingsAtom); + const viewedShortcutLabel = shortcutLabelForCommand(keybindings, "diff.toggleFileViewed", { + context: { diffFocus: true }, + }); + const commentShortcutLabel = shortcutLabelForCommand(keybindings, "diff.comment", { + context: { diffFocus: true, diffLineMode: true }, + }); const codeViewReveal = useCodeViewFileReveal(); const viewerHandleRef = useRef(null); const attachViewer = useStableCallback((handle: AnnotatableCodeViewHandle | null) => { @@ -1536,6 +1556,7 @@ export default function DiffPanel({ selectedLines={review.selectedLines} onSelectedLinesChange={review.onSelectedLinesChange} onDraftClose={review.onDraftClose} + commentShortcutLabel={commentShortcutLabel} codeViewKey={`${codeViewMountKey}:${lazySource ? filePatchScope : "preview"}`} className="h-full min-h-0 overflow-auto" files={codeViewFiles} @@ -1575,6 +1596,7 @@ export default function DiffPanel({ { setFileViewed(fileDiff, viewed); setDiffFileCollapsed(getCachedFileEntry(fileDiff).fileKey, viewed); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 9de690b257e7e..084db2bf966ca 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -20,6 +20,7 @@ import { } from "~/reviewCommentContext"; import { nextFileCommentId } from "../files/fileCommentAnnotations"; +import { Tooltip, TooltipPopup } from "../ui/tooltip"; import { DiffCommentAnnotation } from "./DiffCommentAnnotation"; import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "./StyledDiffCodeView"; @@ -95,6 +96,7 @@ interface AnnotatableCodeViewProps { selectedLines: CodeViewLineSelection | null; onSelectedLinesChange: (selection: CodeViewLineSelection | null) => void; onDraftClose?: () => void; + commentShortcutLabel?: string | null; className?: string; renderCodeViewFooter?: () => ReactNode; unsafeCSSExtra?: string; @@ -121,6 +123,7 @@ export function AnnotatableCodeView({ selectedLines, onSelectedLinesChange, onDraftClose, + commentShortcutLabel, className, renderCodeViewFooter, unsafeCSSExtra, @@ -258,61 +261,83 @@ export function AnnotatableCodeView({ return true; }; useImperativeHandle(reviewRef, () => ({ beginComment })); + const [hoveredCommentButton, setHoveredCommentButton] = useState(null); return ( - - key={codeViewKey} - {...(viewerRef ? { viewerRef } : {})} - {...(className ? { className } : {})} - {...(unsafeCSSExtra ? { unsafeCSSExtra } : {})} - {...(renderHeaderMetadata - ? { - renderHeaderMetadata: (item: CodeViewItem) => - item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, - } - : {})} - {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} - items={items} - selectedLines={selectedLines} - onSelectedLinesChange={onSelectedLinesChange} - onRevealSearchMatch={(item) => onRevealSearchMatch(item.id)} - options={{ - ...options, - enableGutterUtility: draft === null, - enableLineSelection: draft === null, - onGutterUtilityClick: (range, { item }) => { - if (range) beginComment(item.id, range); - }, - }} - renderHeaderFilenameSuffix={(item) => - item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null - } - renderHeaderPrefix={(item) => - item.type === "diff" - ? renderHeaderPrefix(item.fileDiff, item.id, item.collapsed === true) - : null + // The viewer draws its own "+" button inside a shadow root, so its tooltip is anchored from here. +
+ setHoveredCommentButton( + event.nativeEvent + .composedPath() + .find( + (node): node is HTMLElement => + node instanceof HTMLElement && node.hasAttribute("data-utility-button"), + ) ?? null, + ) } - renderAnnotation={(...[annotation]) => { - const hasDraft = annotation.metadata.entries.some((entry) => entry.kind === "draft"); - return ( -
- {annotation.metadata.entries.map((entry) => ( - removeEntry(entry.id)} - onComment={(text) => submitEntry(entry.id, text)} - onDelete={() => removeEntry(entry.id)} - /> - ))} -
- ); - }} - /> + onPointerLeave={() => setHoveredCommentButton(null)} + > + + + {commentShortcutLabel ? `Add comment (${commentShortcutLabel})` : "Add comment"} + + + + key={codeViewKey} + {...(viewerRef ? { viewerRef } : {})} + {...(className ? { className } : {})} + {...(unsafeCSSExtra ? { unsafeCSSExtra } : {})} + {...(renderHeaderMetadata + ? { + renderHeaderMetadata: (item: CodeViewItem) => + item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, + } + : {})} + {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} + items={items} + selectedLines={selectedLines} + onSelectedLinesChange={onSelectedLinesChange} + onRevealSearchMatch={(item) => onRevealSearchMatch(item.id)} + options={{ + ...options, + enableGutterUtility: draft === null, + enableLineSelection: draft === null, + onGutterUtilityClick: (range, { item }) => { + if (range) beginComment(item.id, range); + }, + }} + renderHeaderFilenameSuffix={(item) => + item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null + } + renderHeaderPrefix={(item) => + item.type === "diff" + ? renderHeaderPrefix(item.fileDiff, item.id, item.collapsed === true) + : null + } + renderAnnotation={(...[annotation]) => { + const hasDraft = annotation.metadata.entries.some((entry) => entry.kind === "draft"); + return ( +
+ {annotation.metadata.entries.map((entry) => ( + removeEntry(entry.id)} + onComment={(text) => submitEntry(entry.id, text)} + onDelete={() => removeEntry(entry.id)} + /> + ))} +
+ ); + }} + /> +
); } diff --git a/apps/web/src/components/diffs/diffReviewCursor.test.ts b/apps/web/src/components/diffs/diffReviewCursor.test.ts index 5be7f53aaf314..06dad2ed3bbc8 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.test.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.test.ts @@ -11,7 +11,6 @@ import { stepRow, topToBottomRange, visibleDiffRows, - wholeFileExpansions, } from "./diffReviewCursor"; const lines = (count: number, edit: (line: number) => string) => @@ -107,10 +106,7 @@ describe("nearestGapExpansion", () => { it("finds nothing once the file is fully shown", () => { const expanded = new Map( - wholeFileExpansions(fileDiff, unified, NO_EXPANSIONS).map((expansion) => [ - expansion.hunkIndex, - { fromStart: expansion.lineCount, fromEnd: 0 }, - ]), + [0, 1, 2].map((hunkIndex) => [hunkIndex, { fromStart: 1_000_000, fromEnd: 0 }]), ); const rows = visibleDiffRows(fileDiff, "unified", expanded); expect(rows).toHaveLength(32); @@ -119,26 +115,6 @@ describe("nearestGapExpansion", () => { }); }); -describe("wholeFileExpansions", () => { - it("expands every gap while lines are hidden, then folds back what was expanded", () => { - expect(wholeFileExpansions(fileDiff, unified, NO_EXPANSIONS).map((e) => e.hunkIndex)).toEqual([ - 0, 1, 2, - ]); - const expanded = new Map([ - [0, { fromStart: 1_000_000, fromEnd: 0 }], - [1, { fromStart: 1_000_000, fromEnd: 3 }], - [2, { fromStart: 1_000_000, fromEnd: 0 }], - ]); - const rows = visibleDiffRows(fileDiff, "unified", expanded); - expect(wholeFileExpansions(fileDiff, rows, expanded)).toEqual([ - { hunkIndex: 0, direction: "up", lineCount: -1_000_000 }, - { hunkIndex: 1, direction: "up", lineCount: -1_000_000 }, - { hunkIndex: 1, direction: "down", lineCount: -3 }, - { hunkIndex: 2, direction: "up", lineCount: -1_000_000 }, - ]); - }); -}); - describe("cursor selections", () => { const file = { fileKey: "a", filePath: "a.ts" }; diff --git a/apps/web/src/components/diffs/diffReviewCursor.ts b/apps/web/src/components/diffs/diffReviewCursor.ts index b8908d6f2fbce..1dff8178c8ce3 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.ts @@ -116,40 +116,6 @@ export function nearestGapExpansion( return null; } -// Expansion regions are clamped to their gap when read, so this reveals any gap in full, -// including the trailing gap of a partial diff whose size is unknown until it loads. -const WHOLE_GAP_LINE_COUNT = 1_000_000; - -export interface DiffGapExpansionCount extends DiffGapExpansion { - readonly lineCount: number; -} - -/** - * Shows the whole file while anything is hidden or nothing was expanded yet; otherwise folds back - * exactly what the viewer reports as expanded. - */ -export function wholeFileExpansions( - fileDiff: FileDiffMetadata, - rows: ReadonlyArray, - expandedHunks: ReadonlyMap, -): ReadonlyArray { - const hasHiddenLines = rows.some((row) => row.hiddenBefore > 0 || row.hiddenAfter > 0); - const folds = [...expandedHunks].flatMap(([hunkIndex, region]) => [ - ...(region.fromStart > 0 - ? [{ hunkIndex, direction: "up" as const, lineCount: -region.fromStart }] - : []), - ...(region.fromEnd > 0 - ? [{ hunkIndex, direction: "down" as const, lineCount: -region.fromEnd }] - : []), - ]); - if (!hasHiddenLines && folds.length > 0) return folds; - return Array.from({ length: fileDiff.hunks.length + 1 }, (_, hunkIndex) => ({ - hunkIndex, - direction: "up" as const, - lineCount: WHOLE_GAP_LINE_COUNT, - })); -} - /** 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; diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index ee25cc2718224..dc2d6933edd02 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -22,7 +22,6 @@ import { stepRow, topToBottomRange, visibleDiffRows, - wholeFileExpansions, } from "./diffReviewCursor"; const DIFF_REVIEW_COMMANDS = [ @@ -32,7 +31,6 @@ const DIFF_REVIEW_COMMANDS = [ "diff.previousUnviewedFile", "diff.toggleFileCollapsed", "diff.toggleFileViewed", - "diff.expandFile", "diff.toggleSingleFile", "diff.enterLineMode", "diff.nextLine", @@ -59,6 +57,19 @@ const REPEATABLE_COMMANDS: ReadonlySet = new Set([ const NO_EXPANSIONS: ReadonlyMap = new Map(); +// A control reached with Tab keeps Enter and Space; one focused by a click hands them to the cursor. +function activatesFocusedControl(event: KeyboardEvent, root: HTMLElement | null): boolean { + return ( + (event.key === "Enter" || event.key === " ") && + event.target instanceof Element && + event.target !== root && + event.target.matches(":focus-visible") && + event.target.closest( + 'button, a[href], input, select, [role="button"], [role="checkbox"], [role="switch"]', + ) !== null + ); +} + export interface DiffReviewFile { readonly fileKey: string; readonly filePath: string; @@ -214,22 +225,6 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { moveToFile(nextKey ?? cursorFile.fileKey); return true; } - case "diff.expandFile": { - if (!cursorFile) return false; - if (!cursorFile.rendered || !canExpand(cursorFile)) return true; - input.setCollapsed(cursorFile.fileKey, false); - const instance = fileDiffInstance(cursorFile.fileKey); - if (!instance) return true; - const expansions = wholeFileExpansions( - cursorFile.fileDiff, - rowsFor(cursorFile), - instance.getExpandedHunks(), - ); - for (const { hunkIndex, direction, lineCount } of expansions) { - instance.expandHunk(hunkIndex, direction, lineCount); - } - return true; - } case "diff.toggleSingleFile": { const file = cursorFile ?? files[0]; if (!file) return false; @@ -327,7 +322,9 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { // Capture phase, so the keys reach the panel before type-to-focus sends them to the composer. return registerCommands(handlers, { capture: true, - when: () => rootRef.current?.contains(document.activeElement) === true, + when: (event) => + rootRef.current?.contains(document.activeElement) === true && + !activatesFocusedControl(event, rootRef.current), context: () => ({ diffLineMode: isLineMode() }), }); }, [handleCommand, isLineMode]); diff --git a/apps/web/src/components/settings/KeybindingsSettings.logic.ts b/apps/web/src/components/settings/KeybindingsSettings.logic.ts index 4eace440a51bd..fe9b47b4dcadc 100644 --- a/apps/web/src/components/settings/KeybindingsSettings.logic.ts +++ b/apps/web/src/components/settings/KeybindingsSettings.logic.ts @@ -382,7 +382,6 @@ export function commandLabel(command: KeybindingCommand): string { if (usagePeriod) return `Usage: Period: ${usagePeriod.label}`; if (command === "view.reopenClosed") return "Reopen Closed Tab"; if (command === "preview.focusUrl") return "Preview: Focus URL"; - if (command === "diff.expandFile") return "Diff: Show Whole File"; const raw = String(command); if (raw.startsWith("script.") && raw.endsWith(".run")) { return `Run Script: ${titleCaseCommandSegment(raw.slice("script.".length, -".run".length))}`; diff --git a/apps/web/src/keybindings.test.ts b/apps/web/src/keybindings.test.ts index 3fc981fc453b0..73051dea921ab 100644 --- a/apps/web/src/keybindings.test.ts +++ b/apps/web/src/keybindings.test.ts @@ -938,6 +938,8 @@ describe("Diff review shortcuts", () => { ], [event({ key: "v" }), { diffFocus: true, diffLineMode: true }, "diff.toggleFileViewed"], [event({ key: " ", code: "Space" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], + [event({ key: " ", code: "Space" }), { diffFocus: true }, "diff.toggleFileCollapsed"], + [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) => { diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index b484a347839d2..937ea02454c44 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -74,8 +74,7 @@ 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. - `v` marks the file viewed, which also folds it. Press it again to unmark. -- `o` folds or unfolds the file. -- `x` shows the whole file. Press it again to hide the unchanged lines. +- Enter or `Space` folds or unfolds the file. - `f` shows only the current file. `j`, `k`, and `n` switch which file it shows. On lines: diff --git a/packages/contracts/src/keybindings.ts b/packages/contracts/src/keybindings.ts index 121054f6a478f..a753928371369 100644 --- a/packages/contracts/src/keybindings.ts +++ b/packages/contracts/src/keybindings.ts @@ -79,7 +79,6 @@ export const STATIC_KEYBINDING_COMMANDS = [ "diff.previousUnviewedFile", "diff.toggleFileCollapsed", "diff.toggleFileViewed", - "diff.expandFile", "diff.toggleSingleFile", "diff.enterLineMode", "diff.nextLine", diff --git a/packages/shared/src/keybindings.ts b/packages/shared/src/keybindings.ts index 39e66988483e5..34ec7a82e6958 100644 --- a/packages/shared/src/keybindings.ts +++ b/packages/shared/src/keybindings.ts @@ -168,9 +168,9 @@ export const DEFAULT_KEYBINDINGS: ReadonlyArray = [ { 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: "o", command: "diff.toggleFileCollapsed", when: "diffFocus && !diffLineMode" }, + { key: "enter", command: "diff.toggleFileCollapsed", when: "diffFocus && !diffLineMode" }, + { key: "space", command: "diff.toggleFileCollapsed", when: "diffFocus && !diffLineMode" }, { key: "v", command: "diff.toggleFileViewed", when: "diffFocus" }, - { key: "x", command: "diff.expandFile", when: "diffFocus && !diffLineMode" }, { key: "f", command: "diff.toggleSingleFile", when: "diffFocus && !diffLineMode" }, { key: "i", command: "diff.enterLineMode", when: "diffFocus && !diffLineMode" }, { key: "j", command: "diff.nextLine", when: "diffFocus && diffLineMode" }, From d87f058f55be0398fe7d665e2df8641ac1ed3c5e Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:00:04 -0700 Subject: [PATCH 10/22] fix(web): keep diff review keys in the Diff panel and comment with c A clicked header control kept focus, so the first review key gave it a focus ring, and Enter or Space pressed it instead of the cursor. Chrome marks a clicked control :focus-visible on the next key press, so the panel now tracks whether focus arrived from the keyboard and takes focus back from a clicked control. Letters typed in the panel no longer jump to the composer, and comments move from Space to c. Co-Authored-By: Claude Opus 5.5 --- .../components/chat/composerFocusRedirect.ts | 2 + .../chat/useChatShortcuts.browser.test.tsx | 15 ++++ .../useDiffReviewKeyboard.browser.test.tsx | 75 +++++++++++++++++++ .../components/diffs/useDiffReviewKeyboard.ts | 55 ++++++++++---- apps/web/src/keybindings.test.ts | 3 +- docs/user/keybindings.md | 4 +- packages/shared/src/keybindings.ts | 2 +- 7 files changed, 137 insertions(+), 19 deletions(-) create mode 100644 apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx diff --git a/apps/web/src/components/chat/composerFocusRedirect.ts b/apps/web/src/components/chat/composerFocusRedirect.ts index f7730b367925e..bb87c2dc5ffa4 100644 --- a/apps/web/src/components/chat/composerFocusRedirect.ts +++ b/apps/web/src/components/chat/composerFocusRedirect.ts @@ -75,6 +75,8 @@ export function shouldTypeToFocusComposer(event: KeyboardEvent): boolean { if (event.key.length !== 1) return false; if (!shouldRedirectInputToComposer(event)) return false; if (isSurfaceLauncherKey(event)) return false; + // The Diff panel reads letters as review keys, bound or not. + if (eventPathContainsSelector(event, "[data-diff-panel]")) return false; return true; } diff --git a/apps/web/src/components/chat/useChatShortcuts.browser.test.tsx b/apps/web/src/components/chat/useChatShortcuts.browser.test.tsx index d04247d279792..79f58b76fe927 100644 --- a/apps/web/src/components/chat/useChatShortcuts.browser.test.tsx +++ b/apps/web/src/components/chat/useChatShortcuts.browser.test.tsx @@ -98,6 +98,21 @@ it("still types other letters into the composer while the launcher is open", asy expect(received).toEqual([]); }); +it("leaves letters typed in the Diff panel out of the composer", async () => { + 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/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx new file mode 100644 index 0000000000000..7a0e80c6cd5cd --- /dev/null +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -0,0 +1,75 @@ +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 { useDiffReviewKeyboard } 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, +}; + +function Panel(props: { folds: string[]; clicks: string[] }) { + const { rootRef } = useDiffReviewKeyboard({ + scopeKey: "scope", + files: [file], + diffStyle: "unified", + canLoadFullFiles: false, + autoAdvanceOnViewed: false, + getViewer: () => null, + isCollapsed: () => false, + setCollapsed: (fileKey) => void props.folds.push(fileKey), + isViewed: () => false, + setViewed: () => {}, + revealFile: () => {}, + singleFile: false, + setSingleFile: () => {}, + }); + return ( +
+ +
+ ); +} + +// 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", "a"]); + expect(clicks).toEqual(["Changes", "Changes"]); +}); + +it("leaves Enter and Space 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", "Changes"]); +}); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index dc2d6933edd02..bfc8f7ba3108d 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -57,17 +57,11 @@ const REPEATABLE_COMMANDS: ReadonlySet = new Set([ const NO_EXPANSIONS: ReadonlyMap = new Map(); -// A control reached with Tab keeps Enter and Space; one focused by a click hands them to the cursor. -function activatesFocusedControl(event: KeyboardEvent, root: HTMLElement | null): boolean { - return ( - (event.key === "Enter" || event.key === " ") && - event.target instanceof Element && - event.target !== root && - event.target.matches(":focus-visible") && - event.target.closest( - 'button, a[href], input, select, [role="button"], [role="checkbox"], [role="switch"]', - ) !== null - ); +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; } export interface DiffReviewFile { @@ -111,6 +105,8 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { const rootRef = useRef(null); const reviewRef = useRef(null); 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); const storedCursor = cursorState.scopeKey === scopeKey ? cursorState.cursor : null; const cursorFile = storedCursor @@ -308,6 +304,10 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { event.preventDefault(); // The viewer drops a scroll requested during a key press that reaches it. event.stopPropagation(); + // Otherwise a clicked header control shows a focus ring while the cursor moves. + if (isControlFocused() && document.activeElement !== tabbedToRef.current) { + rootRef.current?.focus({ preventScroll: true }); + } } return handled; }); @@ -319,14 +319,39 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { 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. - return registerCommands(handlers, { + const unregister = registerCommands(handlers, { capture: true, - when: (event) => - rootRef.current?.contains(document.activeElement) === true && - !activatesFocusedControl(event, rootRef.current), + when: (event) => { + if (rootRef.current?.contains(document.activeElement) !== true) return false; + // A control reached from the keyboard keeps Enter and Space; a clicked one hands them over. + return !( + (event.key === "Enter" || event.key === " ") && + 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]); return { diff --git a/apps/web/src/keybindings.test.ts b/apps/web/src/keybindings.test.ts index 73051dea921ab..3b2da43241f45 100644 --- a/apps/web/src/keybindings.test.ts +++ b/apps/web/src/keybindings.test.ts @@ -937,7 +937,8 @@ describe("Diff review shortcuts", () => { "diff.startSelection", ], [event({ key: "v" }), { diffFocus: true, diffLineMode: true }, "diff.toggleFileViewed"], - [event({ key: " ", code: "Space" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], + [event({ key: "c" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], + [event({ key: " ", code: "Space" }), { diffFocus: true, diffLineMode: true }, null], [event({ key: " ", code: "Space" }), { diffFocus: true }, "diff.toggleFileCollapsed"], [event({ key: "Enter" }), { diffFocus: true }, "diff.toggleFileCollapsed"], [event({ key: "Escape" }), { diffFocus: true, diffLineMode: true }, "diff.back"], diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 937ea02454c44..2f12d8a5d72d1 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -81,9 +81,9 @@ On lines: - `j` and `k` move one line. - `Shift+j` and `Shift+k` show more of the unchanged lines below or above. -- `Space` comments on the line. After you send or cancel the comment, you are +- `c` comments on the line. After you send or cancel the comment, you are back on the line. -- `Shift+v` starts a selection. `j` and `k` extend it, and `Space` comments on +- `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. diff --git a/packages/shared/src/keybindings.ts b/packages/shared/src/keybindings.ts index 34ec7a82e6958..cb66804f58a87 100644 --- a/packages/shared/src/keybindings.ts +++ b/packages/shared/src/keybindings.ts @@ -177,7 +177,7 @@ export const DEFAULT_KEYBINDINGS: ReadonlyArray = [ { 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: "space", command: "diff.comment", 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" }, From bfefeff2e993c8af0ea4e03e020cf2992d01b8c7 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:17:29 -0700 Subject: [PATCH 11/22] feat(web): review pull request diffs from the keyboard The review keys only worked in the Diff panel; the PR Code tab had its own fold, viewed, and comment code and no keyboard path. Both views now share useDiffReviewKeyboard, which owns the cursor, the single-file preference, and the shortcut labels, plus shared controls for the cursor outline, the Viewed checkbox, and the single-file toggle. The gutter "+" tooltip moves into StyledDiffCodeView so every diff view gets it. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/DiffPanel.tsx | 109 ++------ .../components/diffs/AnnotatableCodeView.tsx | 132 ++++------ .../components/diffs/DiffReviewControls.tsx | 98 +++++++ .../components/diffs/StyledDiffCodeView.tsx | 89 +++++-- .../useDiffReviewKeyboard.browser.test.tsx | 3 +- .../components/diffs/useDiffReviewKeyboard.ts | 46 ++-- .../pullRequest/PullRequestCodeTab.tsx | 240 ++++++++++-------- docs/user/keybindings.md | 14 +- 8 files changed, 415 insertions(+), 316 deletions(-) create mode 100644 apps/web/src/components/diffs/DiffReviewControls.tsx diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index b052ddee819ce..c9117ca99e97a 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -13,7 +13,6 @@ import { CheckIcon, ChevronDownIcon, Columns2Icon, - FileIcon, FlaskConicalOffIcon, FolderTreeIcon, PilcrowIcon, @@ -68,7 +67,16 @@ 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, + DiffViewedToggle, +} from "./diffs/DiffReviewControls"; import { type DiffReviewFile, useDiffReviewKeyboard } from "./diffs/useDiffReviewKeyboard"; import { DiffFileTree } from "./diffs/DiffFileTree"; import { FileExplorerPane } from "./files/FileExplorerPane"; @@ -77,7 +85,6 @@ import { Button } from "./ui/button"; import { MorphIcon } from "~/components/MorphIcon"; import { ToggleGroup, Toggle } from "./ui/toggle-group"; import { Switch } from "./ui/switch"; -import { Checkbox } from "./ui/checkbox"; import { Combobox, ComboboxEmpty, @@ -101,8 +108,7 @@ import { import { Tooltip, TooltipPopup, TooltipTrigger } from "./ui/tooltip"; import { useEnvironmentQuery } from "../state/query"; import { useAtomCommand } from "../state/use-atom-command"; -import { primaryServerKeybindingsAtom, serverEnvironment } from "../state/server"; -import { shortcutLabelForCommand } from "../keybindings"; +import { serverEnvironment } from "../state/server"; import { reviewEnvironment } from "../state/review"; import { vcsEnvironment } from "../state/vcs"; import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices"; @@ -118,7 +124,6 @@ import { useOpenLink } from "../browser/useOpenLink"; type DiffThemeType = "light" | "dark"; const AUTOMATIC_BASE_REF = "__automatic_base_ref__"; const DIFF_FILE_TREE_STORAGE_KEY = "t3code.diffFileTreeOpen"; -const DIFF_SINGLE_FILE_STORAGE_KEY = "t3code.diffSingleFile"; const fileEntryCache = new WeakMap< FileDiffMetadata, { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number; viewedFingerprint: string } @@ -156,13 +161,7 @@ function DiffFileCollapseToggle({ }) { return ( <> - {hasCursor ? ( - // The sticky header is the nearest positioned box, so this outlines the whole header. - - ) : null} + {hasCursor ? : null} void; -}) { - const action = viewed ? "Mark as not viewed" : "Mark as viewed"; - return ( - - - } - > - onViewedChange(checked)} - /> - Viewed - - - {shortcutLabel ? `${action} (${shortcutLabel})` : action} - - - ); -} - function DiffFileHeaderSuffix({ filePath, hasStat, @@ -279,11 +245,6 @@ export default function DiffPanel({ false, Schema.Boolean, ); - const [singleFile, setSingleFile] = useLocalStorage( - DIFF_SINGLE_FILE_STORAGE_KEY, - false, - Schema.Boolean, - ); const [hideTestFiles, setHideTestFiles] = useHideTestFiles(); const [baseRefQuery, setBaseRefQuery] = useState(""); // The open commit menu keeps the list it opened with, so rows do not move under the pointer. @@ -736,13 +697,6 @@ export default function DiffPanel({ rendered: isFileReady(fileDiff) && !isFilePending(fileDiff), })); const viewedFileCount = shownFileEntries.filter(({ fileDiff }) => isFileViewed(fileDiff)).length; - const keybindings = useAtomValue(primaryServerKeybindingsAtom); - const viewedShortcutLabel = shortcutLabelForCommand(keybindings, "diff.toggleFileViewed", { - context: { diffFocus: true }, - }); - const commentShortcutLabel = shortcutLabelForCommand(keybindings, "diff.comment", { - context: { diffFocus: true, diffLineMode: true }, - }); const codeViewReveal = useCodeViewFileReveal(); const viewerHandleRef = useRef(null); const attachViewer = useStableCallback((handle: AnnotatableCodeViewHandle | null) => { @@ -757,11 +711,8 @@ export default function DiffPanel({ } codeViewReveal.request(file.fileKey); }; - const { - rootRef: reviewRootRef, - reviewRef, - ...review - } = useDiffReviewKeyboard({ + const reviewRef = useRef(null); + const { rootRef: reviewRootRef, ...review } = useDiffReviewKeyboard({ scopeKey: collapseScopeKey, files: reviewFiles, diffStyle: diffLayout === "split" ? "split" : "unified", @@ -774,8 +725,7 @@ export default function DiffPanel({ setViewed: (file, viewed) => setFileViewed(file.fileDiff, viewed), revealFile: (file) => revealFileAt(renderableFileEntries.findIndex(({ fileKey }) => fileKey === file.fileKey)), - singleFile, - setSingleFile, + beginComment: (fileKey, range) => reviewRef.current?.beginComment(fileKey, range), }); const renderLoadingBoundary = () => review.singleFileKey === undefined && settledFileCount < renderableFiles.length ? ( @@ -1300,24 +1250,11 @@ export default function DiffPanel({ {diffFileKeys.length > 0 && ( - - - } - > - - - - {singleFile ? "Show all files" : "Show one file at a time"} - - + )} ) : null} {isFilePending(fileDiff) ? null : ( - { setFileViewed(fileDiff, viewed); setDiffFileCollapsed(getCachedFileEntry(fileDiff).fileKey, viewed); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 084db2bf966ca..83616f60587da 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -20,7 +20,6 @@ import { } from "~/reviewCommentContext"; import { nextFileCommentId } from "../files/fileCommentAnnotations"; -import { Tooltip, TooltipPopup } from "../ui/tooltip"; import { DiffCommentAnnotation } from "./DiffCommentAnnotation"; import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "./StyledDiffCodeView"; @@ -261,83 +260,64 @@ export function AnnotatableCodeView({ return true; }; useImperativeHandle(reviewRef, () => ({ beginComment })); - const [hoveredCommentButton, setHoveredCommentButton] = useState(null); return ( - // The viewer draws its own "+" button inside a shadow root, so its tooltip is anchored from here. -
- setHoveredCommentButton( - event.nativeEvent - .composedPath() - .find( - (node): node is HTMLElement => - node instanceof HTMLElement && node.hasAttribute("data-utility-button"), - ) ?? null, - ) + + key={codeViewKey} + gutterUtilityLabel={ + commentShortcutLabel ? `Add comment (${commentShortcutLabel})` : "Add comment" } - onPointerLeave={() => setHoveredCommentButton(null)} - > - - - {commentShortcutLabel ? `Add comment (${commentShortcutLabel})` : "Add comment"} - - - - key={codeViewKey} - {...(viewerRef ? { viewerRef } : {})} - {...(className ? { className } : {})} - {...(unsafeCSSExtra ? { unsafeCSSExtra } : {})} - {...(renderHeaderMetadata - ? { - renderHeaderMetadata: (item: CodeViewItem) => - item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, - } - : {})} - {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} - items={items} - selectedLines={selectedLines} - onSelectedLinesChange={onSelectedLinesChange} - onRevealSearchMatch={(item) => onRevealSearchMatch(item.id)} - options={{ - ...options, - enableGutterUtility: draft === null, - enableLineSelection: draft === null, - onGutterUtilityClick: (range, { item }) => { - if (range) beginComment(item.id, range); - }, - }} - renderHeaderFilenameSuffix={(item) => - item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null - } - renderHeaderPrefix={(item) => - item.type === "diff" - ? renderHeaderPrefix(item.fileDiff, item.id, item.collapsed === true) - : null - } - renderAnnotation={(...[annotation]) => { - const hasDraft = annotation.metadata.entries.some((entry) => entry.kind === "draft"); - return ( -
- {annotation.metadata.entries.map((entry) => ( - removeEntry(entry.id)} - onComment={(text) => submitEntry(entry.id, text)} - onDelete={() => removeEntry(entry.id)} - /> - ))} -
- ); - }} - /> -
+ {...(viewerRef ? { viewerRef } : {})} + {...(className ? { className } : {})} + {...(unsafeCSSExtra ? { unsafeCSSExtra } : {})} + {...(renderHeaderMetadata + ? { + renderHeaderMetadata: (item: CodeViewItem) => + item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, + } + : {})} + {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} + items={items} + selectedLines={selectedLines} + onSelectedLinesChange={onSelectedLinesChange} + onRevealSearchMatch={(item) => onRevealSearchMatch(item.id)} + options={{ + ...options, + enableGutterUtility: draft === null, + enableLineSelection: draft === null, + onGutterUtilityClick: (range, { item }) => { + if (range) beginComment(item.id, range); + }, + }} + renderHeaderFilenameSuffix={(item) => + item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null + } + renderHeaderPrefix={(item) => + item.type === "diff" + ? renderHeaderPrefix(item.fileDiff, item.id, item.collapsed === true) + : null + } + renderAnnotation={(...[annotation]) => { + const hasDraft = annotation.metadata.entries.some((entry) => entry.kind === "draft"); + return ( +
+ {annotation.metadata.entries.map((entry) => ( + removeEntry(entry.id)} + onComment={(text) => submitEntry(entry.id, text)} + onDelete={() => removeEntry(entry.id)} + /> + ))} +
+ ); + }} + /> ); } diff --git a/apps/web/src/components/diffs/DiffReviewControls.tsx b/apps/web/src/components/diffs/DiffReviewControls.tsx new file mode 100644 index 0000000000000..75300fcc44d68 --- /dev/null +++ b/apps/web/src/components/diffs/DiffReviewControls.tsx @@ -0,0 +1,98 @@ +import { FileIcon } from "lucide-react"; + +import { Checkbox } from "../ui/checkbox"; +import { Toggle } from "../ui/toggle-group"; +import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; + +const withShortcut = (label: string, shortcut: string | null) => + shortcut ? `${label} (${shortcut})` : label; + +/** Outlines the file header under the review cursor; render it in the header prefix. */ +export function DiffCursorOutline() { + 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 99049e280cb5a..40fc36f1ee1c6 100644 --- a/apps/web/src/components/diffs/StyledDiffCodeView.tsx +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -7,10 +7,11 @@ 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"; 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 @@ -215,6 +216,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({ @@ -222,37 +225,67 @@ export function StyledDiffCodeView({ viewerRef, className, unsafeCSSExtra, + gutterUtilityLabel, ...props }: StyledDiffCodeViewProps) { + const [hoveredUtilityButton, setHoveredUtilityButton] = useState(null); + const view = ( + + {...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 }, + }} + /> + ); 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/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 7a0e80c6cd5cd..05e81342cfdcf 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -31,8 +31,7 @@ function Panel(props: { folds: string[]; clicks: string[] }) { isViewed: () => false, setViewed: () => {}, revealFile: () => {}, - singleFile: false, - setSingleFile: () => {}, + beginComment: () => {}, }); return (
diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index bfc8f7ba3108d..ae237fe7dac0a 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -1,16 +1,22 @@ +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 { useEffect, useRef, useState } from "react"; +import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useStableCallback } from "~/hooks/useStableCallback"; +import { shortcutLabelForCommand } from "~/keybindings"; import { type CommandHandlers, registerCommands } from "~/shortcuts/dispatcher"; +import { primaryServerKeybindingsAtom } from "~/state/server"; -import type { AnnotatableCodeViewHandle, AnnotatableCodeViewReview } from "./AnnotatableCodeView"; import { adjacentFileKey, adjacentUnviewedFileKey, @@ -56,6 +62,7 @@ const REPEATABLE_COMMANDS: ReadonlySet = new Set([ ]); const NO_EXPANSIONS: ReadonlyMap = new Map(); +const SINGLE_FILE_STORAGE_KEY = "t3code.diffSingleFile"; const CONTROL_SELECTOR = 'button, a[href], input, select, [role="button"], [role="checkbox"], [role="switch"]'; @@ -72,7 +79,7 @@ export interface DiffReviewFile { readonly rendered: boolean; } -interface DiffReviewKeyboardInput { +interface DiffReviewKeyboardInput { /** The thread and diff scope; a new one starts without a cursor. */ readonly scopeKey: string | null; readonly files: ReadonlyArray; @@ -80,30 +87,34 @@ interface DiffReviewKeyboardInput { /** Partial diffs only expand when the full files can be loaded. */ readonly canLoadFullFiles: boolean; readonly autoAdvanceOnViewed: boolean; - readonly getViewer: () => AnnotatableCodeViewHandle | null; + readonly getViewer: () => CodeViewHandle | null; readonly isCollapsed: (fileKey: string) => boolean; readonly setCollapsed: (fileKey: string, collapsed: boolean) => void; readonly isViewed: (file: DiffReviewFile) => boolean; readonly setViewed: (file: DiffReviewFile, viewed: boolean) => void; /** Loads the file if needed and scrolls its header into view. */ readonly revealFile: (file: DiffReviewFile) => void; - /** Shows only the cursor's file, forced open. */ - readonly singleFile: boolean; - readonly setSingleFile: (singleFile: boolean) => 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 inside the panel. */ -export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { +export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { const { scopeKey, files } = input; const [cursorState, setCursorState] = useState<{ readonly scopeKey: string | null; readonly cursor: DiffReviewCursor | null; }>({ scopeKey, cursor: null }); const rootRef = useRef(null); - const reviewRef = 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); @@ -114,7 +125,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { files.find((file) => file.filePath === storedCursor.filePath) ?? null) : null; - const singleFileKey = input.singleFile ? (cursorFile ?? files[0])?.fileKey : undefined; + const singleFileKey = singleFile ? (cursorFile ?? files[0])?.fileKey : undefined; const isCollapsed = (fileKey: string) => fileKey !== singleFileKey && input.isCollapsed(fileKey); const cursor: DiffReviewCursor | null = storedCursor === null || cursorFile === null @@ -225,7 +236,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { const file = cursorFile ?? files[0]; if (!file) return false; setCursor(fileCursor(file)); - input.setSingleFile(!input.singleFile); + setSingleFile(!singleFile); input.revealFile(file); return true; } @@ -268,10 +279,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { } case "diff.comment": { if (!cursor || !cursorFile || !range) return false; - reviewRef.current?.beginComment( - cursorFile.fileKey, - topToBottomRange(range, rowsFor(cursorFile)), - ); + input.beginComment(cursorFile.fileKey, topToBottomRange(range, rowsFor(cursorFile))); return true; } case "diff.startSelection": { @@ -354,13 +362,21 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { }; }, [handleCommand, isLineMode]); + const labelFor = (command: DiffReviewCommand, diffLineMode = false) => + shortcutLabelForCommand(keybindings, command, { context: { diffFocus: true, diffLineMode } }); + return { cursor, + singleFile, singleFileKey, selectedLines, rootRef, - reviewRef, flushScroll, + shortcutLabels: { + viewed: labelFor("diff.toggleFileViewed"), + comment: labelFor("diff.comment", true), + singleFile: labelFor("diff.toggleSingleFile"), + }, toggleSingleFile: () => runCommand("diff.toggleSingleFile"), /** Puts the file cursor on a file the user picked another way, such as the file tree. */ selectFile: (filePath: string) => { diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 8246fad432b43..2643665934824 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -3,7 +3,7 @@ import { sourceControlClients, } from "@t3tools/client-runtime/source-control-clients"; import type { CodeViewItem, DiffLineAnnotation, 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, @@ -83,12 +83,17 @@ import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; import { DiffFileTree } from "../diffs/DiffFileTree"; import { FileExplorerPane } from "../files/FileExplorerPane"; import { useCodeViewFileReveal } from "../diffs/useCodeViewFileReveal"; +import { + DiffCursorOutline, + DiffSingleFileToggle, + DiffViewedToggle, +} from "../diffs/DiffReviewControls"; +import { useDiffReviewKeyboard } 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 { MorphIcon } from "~/components/MorphIcon"; -import { Checkbox } from "../ui/checkbox"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { DropdownMenu, @@ -181,6 +186,7 @@ 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; } // Context updates viewed toggles without invalidating memoized header portals. @@ -197,40 +203,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) + } + /> ); } @@ -293,6 +277,7 @@ function PullRequestCodeTab({ useDiffWorkerPool(); const settings = useClientSettings((settings) => ({ diffFilesCollapsed: settings.diffFilesCollapsed, + diffAutoAdvanceOnViewed: settings.diffAutoAdvanceOnViewed, diffIgnoreWhitespace: settings.diffIgnoreWhitespace, diffLayout: settings.diffLayout, wordWrap: settings.wordWrap, @@ -312,10 +297,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); @@ -341,7 +322,6 @@ function PullRequestCodeTab({ setStateScopeKey(scopeKey); setLastViewedFileKey(null); setDraft(null); - setSelectedLines(null); setToggledFiles(new Set()); setFoldOverride(null); setVisibleCommitCount(COMMIT_PAGE_SIZE); @@ -665,9 +645,66 @@ function PullRequestCodeTab({
); - const items: CodeViewDiffItem[] = annotatedFiles.map( - ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { - const collapsed = isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + const isCollapsed = (fileKey: string) => + isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + const setCollapsed = (fileKey: string, collapsed: boolean) => + setToggledFiles((current) => + toggleFileDiffFoldForViewed(fileKey, collapsed, effectiveFoldOverride, current), + ); + const toggleFile = (fileKey: string) => setCollapsed(fileKey, !isCollapsed(fileKey)); + const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { + const range = getFileViewedRange( + annotatedFiles, + fileKey, + extendRange ? lastViewedFileKey : null, + ); + setLastViewedFileKey(fileKey); + for (const file of range) setViewed(file.path, viewed); + setToggledFiles((current) => + range.reduce( + (next, file) => + toggleFileDiffFoldForViewed(file.fileKey, viewed, effectiveFoldOverride, next), + current, + ), + ); + }; + const viewerHandleRef = useRef | null>(null); + const attachViewer = (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: (file, viewed) => { + if (filesViewedEnabled) setFileViewed(file.fileKey, viewed, false); + }, + revealFile: (file) => codeViewReveal.request(file.fileKey), + 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" : ""}` : ""; @@ -679,8 +716,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), @@ -708,47 +744,21 @@ function PullRequestCodeTab({ ); }; - const toggleFile = (fileKey: string) => - setToggledFiles((current) => { - const next = new Set(current); - if (next.has(fileKey)) next.delete(fileKey); - else next.add(fileKey); - return next; - }); - - const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { - const range = getFileViewedRange( - annotatedFiles, - fileKey, - extendRange ? lastViewedFileKey : null, - ); - setLastViewedFileKey(fileKey); - for (const file of range) setViewed(file.path, viewed); - setToggledFiles((current) => - range.reduce( - (next, file) => - toggleFileDiffFoldForViewed(file.fileKey, viewed, effectiveFoldOverride, next), - current, - ), - ); - }; const fileViewedToggle = { enabled: filesViewedEnabled, isViewed: isFileViewed, isStale: isFileViewedStale, setFileViewed, + shortcutLabel: diffReview.shortcutLabels.viewed, }; const revealFile = (path: string) => { - 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; + diffReview.selectFile(path); // Unfold only: a second reveal before the unfold renders must not fold the file again. - if (item.collapsed === true) { - setToggledFiles((current) => - toggleFileDiffFoldForViewed(item.id, false, effectiveFoldOverride, current), - ); - } - codeViewReveal.request(item.id); + setCollapsed(file.fileKey, false); + codeViewReveal.request(file.fileKey); }; const toggleAllFiles = () => { @@ -790,6 +800,11 @@ function PullRequestCodeTab({ }); }; + const closeDraft = () => { + setDraft(null); + diffReview.onDraftClose(); + }; + const finishSelection = ( anchor: DraftAnchor, text: string, @@ -808,8 +823,7 @@ function PullRequestCodeTab({ range: anchor.range, text, }); - setDraft(null); - setSelectedLines(null); + closeDraft(); if (comment !== null) onFinish(comment); }; @@ -836,19 +850,22 @@ function PullRequestCodeTab({ const renderHeaderPrefix = (item: CodeViewItem) => { const collapsed = item.collapsed === true; return ( - + <> + {diffReview.cursor?.fileKey === item.id ? : null} + + ); }; @@ -888,7 +905,10 @@ function PullRequestCodeTab({ enableLineSelection: canSelectLines && draft === null, onGutterUtilityClick: beginComment, onLineSelectionEnd: beginComment, - onPostRender: codeViewReveal.flush, + onPostRender: () => { + codeViewReveal.flush(); + diffReview.flushScroll(); + }, }; const runThreadCommand = async ( @@ -998,10 +1018,7 @@ function PullRequestCodeTab({ }, } : {})} - onCancel={() => { - setDraft(null); - setSelectedLines(null); - }} + onCancel={closeDraft} onComment={(body) => { if (!canCommentOnLines) { if (onAddToAgentSelection) { @@ -1019,8 +1036,7 @@ function PullRequestCodeTab({ position: draft.position, body, }); - setDraft(null); - setSelectedLines(null); + closeDraft(); }} /> ) : null} @@ -1032,7 +1048,7 @@ function PullRequestCodeTab({ const changeIgnoreWhitespace = (next: boolean) => { setIgnoreWhitespace(next); setDraft(null); - setSelectedLines(null); + diffReview.onSelectedLinesChange(null); }; const toolbar = (
@@ -1326,6 +1342,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; } @@ -1412,7 +1435,13 @@ function PullRequestCodeTab({ } return ( -
+ // Focusable so a click anywhere in the tab, toolbar included, gives it the review keys. +
{toolbar} {orphanFiles.size > 0 ? (
@@ -1496,10 +1525,15 @@ function PullRequestCodeTab({ className="@container h-full overflow-auto [scrollbar-gutter:stable]" containerRef={scrollerRef} - viewerRef={codeViewReveal.attach} + viewerRef={attachViewer} items={items} - selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} + selectedLines={diffReview.selectedLines} + onSelectedLinesChange={diffReview.onSelectedLinesChange} + gutterUtilityLabel={ + diffReview.shortcutLabels.comment + ? `Add comment (${diffReview.shortcutLabels.comment})` + : "Add comment" + } options={diffViewOptions} renderCodeViewFooter={renderCodeViewFooter} renderHeaderPrefix={renderHeaderPrefix} diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 2f12d8a5d72d1..755cb42c89f69 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -66,8 +66,8 @@ find only searches the files loaded so far. ## Review a diff from the keyboard -Click into the Diff panel to review it without the mouse. The panel starts on -files. Press `i` to move into a file's lines, and Escape to step back out. +Click into the Diff panel or a pull request's Code tab to review it without the +mouse. Both start on files. Press `i` to move into a file's lines, and Escape to step back out. On files: @@ -81,14 +81,16 @@ On lines: - `j` and `k` move one line. - `Shift+j` and `Shift+k` show more of the unchanged lines below or above. -- `c` comments on the line. After you send or cancel the comment, you are - back on the line. +- `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. -Viewed marks belong to this device and thread. A file that changes after you -view it shows as not viewed again. To jump to the next unviewed file after each +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**. Change any of these keys in **Settings → Keybindings** under the **Diff** From 1c317f13a47cf36ef57c3e2c00a7cb0a6c2e05c1 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 18:47:08 -0700 Subject: [PATCH 12/22] fix(web): diff review keys work from anywhere in the diff's panel The keys now run while a tab or button elsewhere in the panel has focus, such as a pull request's Code tab, and move focus off any control they pass. Diff keybindings get their own settings section. Co-Authored-By: Claude Opus 5.5 --- .../useDiffReviewKeyboard.browser.test.tsx | 41 +++++++++++++++++-- .../components/diffs/useDiffReviewKeyboard.ts | 12 +++--- .../settings/KeybindingsSettings.logic.ts | 3 +- apps/web/src/lib/diffFocus.ts | 13 ++++-- docs/user/keybindings.md | 3 +- 5 files changed, 58 insertions(+), 14 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 05e81342cfdcf..077234e90763b 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -18,7 +18,7 @@ const file = { rendered: true, }; -function Panel(props: { folds: string[]; clicks: string[] }) { +function Panel(props: { folds: string[]; clicks: string[]; reveals?: string[]; hidden?: boolean }) { const { rootRef } = useDiffReviewKeyboard({ scopeKey: "scope", files: [file], @@ -30,11 +30,17 @@ function Panel(props: { folds: string[]; clicks: string[] }) { setCollapsed: (fileKey) => void props.folds.push(fileKey), isViewed: () => false, setViewed: () => {}, - revealFile: () => {}, + revealFile: (revealed) => void props.reveals?.push(revealed.fileKey), beginComment: () => {}, }); return ( -
+
@@ -60,7 +66,7 @@ it("takes the keys from a clicked control and drops its focus ring", async () => expect(clicks).toEqual(["Changes", "Changes"]); }); -it("leaves Enter and Space to a control reached with Tab", async () => { +it("leaves only Enter and Space to a control reached with Tab", async () => { const folds: string[] = []; const clicks: string[] = []; await using screen = await render(, options); @@ -71,4 +77,31 @@ it("leaves Enter and Space to a control reached with Tab", async () => { expect(folds).toEqual([]); expect(clicks).toEqual(["Changes", "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([]); }); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index ae237fe7dac0a..85c35e7a6a92a 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -14,6 +14,7 @@ import { useEffect, useRef, useState } 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"; @@ -99,7 +100,7 @@ interface DiffReviewKeyboardInput { /** * 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 inside the panel. + * selection. Commands run through the shortcut dispatcher while focus is in the diff's panel. */ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { const { scopeKey, files } = input; @@ -312,9 +313,10 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput event.preventDefault(); // The viewer drops a scroll requested during a key press that reaches it. event.stopPropagation(); - // Otherwise a clicked header control shows a focus ring while the cursor moves. - if (isControlFocused() && document.activeElement !== tabbedToRef.current) { - rootRef.current?.focus({ preventScroll: true }); + // 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; @@ -344,7 +346,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput const unregister = registerCommands(handlers, { capture: true, when: (event) => { - if (rootRef.current?.contains(document.activeElement) !== true) return false; + if (!rootRef.current || !diffPanelHasFocus(rootRef.current)) return false; // A control reached from the keyboard keeps Enter and Space; a clicked one hands them over. return !( (event.key === "Enter" || event.key === " ") && diff --git a/apps/web/src/components/settings/KeybindingsSettings.logic.ts b/apps/web/src/components/settings/KeybindingsSettings.logic.ts index fe9b47b4dcadc..c79c7fe08971a 100644 --- a/apps/web/src/components/settings/KeybindingsSettings.logic.ts +++ b/apps/web/src/components/settings/KeybindingsSettings.logic.ts @@ -216,7 +216,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/lib/diffFocus.ts b/apps/web/src/lib/diffFocus.ts index 8c2feeb5a2aad..fba3b9ce12ea8 100644 --- a/apps/web/src/lib/diffFocus.ts +++ b/apps/web/src/lib/diffFocus.ts @@ -1,7 +1,14 @@ import { isEditableFocused } from "./editableFocus"; -export function isDiffFocused(): boolean { +/** 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 instanceof HTMLElement) || !activeElement.isConnected) return false; - return activeElement.closest("[data-diff-panel]") !== null && !isEditableFocused(); + 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/docs/user/keybindings.md b/docs/user/keybindings.md index 755cb42c89f69..acf8ab4e1e17f 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -67,7 +67,8 @@ 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. Both start on files. Press `i` to move into a file's lines, and Escape to step back out. +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: From 0748b5a086b64adc940836e4ca91c32cded12186 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:03:39 -0700 Subject: [PATCH 13/22] fix(web): diff file keys start from the file on screen after scrolling Scrolling away from the review cursor and pressing j or k jumped back to the file next to the old cursor, often near the top. When the cursor's file is off screen, j, k, n, and Shift+N now start from the file at the top of the view. Co-Authored-By: Claude Opus 5.5 --- .../components/diffs/diffReviewCursor.test.ts | 13 +++++++++ .../src/components/diffs/diffReviewCursor.ts | 29 +++++++++++++++++++ .../components/diffs/useDiffReviewKeyboard.ts | 26 +++++++++++++++-- docs/user/keybindings.md | 1 + 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/diffs/diffReviewCursor.test.ts b/apps/web/src/components/diffs/diffReviewCursor.test.ts index 06dad2ed3bbc8..e9a461125c037 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.test.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.test.ts @@ -7,6 +7,7 @@ import { cursorFromSelectedRange, cursorSelectedRange, type DiffRow, + fileToStartFrom, nearestGapExpansion, stepRow, topToBottomRange, @@ -170,4 +171,16 @@ describe("file navigation", () => { 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(); + }); }); diff --git a/apps/web/src/components/diffs/diffReviewCursor.ts b/apps/web/src/components/diffs/diffReviewCursor.ts index 1dff8178c8ce3..3f0016ceba790 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.ts @@ -147,6 +147,35 @@ export function cursorFromSelectedRange( 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; +} + /** The neighbouring file, stopping at the ends; with no current file, the first. */ export function adjacentFileKey( fileKeys: ReadonlyArray, diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index 85c35e7a6a92a..cef999d0fb577 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -25,6 +25,7 @@ import { cursorSelectedRange, type DiffReviewCursor, type DiffRowRef, + fileToStartFrom, nearestGapExpansion, stepRow, topToBottomRange, @@ -189,6 +190,21 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput input.revealFile(file); }; + // After the reader scrolls away from the cursor, the file keys start from what is on screen. + const fileKeyInView = () => { + const viewer = input.getViewer()?.getInstance(); + if (!viewer) return null; + const top = viewer.getScrollTop(); + return fileToStartFrom( + files.flatMap(({ fileKey }) => { + const fileTop = viewer.getTopForItem(fileKey); + return fileTop === undefined ? [] : [{ fileKey, top: fileTop }]; + }), + cursorFile?.fileKey ?? null, + { top, bottom: top + viewer.getHeight(), contentEnd: viewer.getScrollHeight() }, + ); + }; + const runCommand = (command: DiffReviewCommand): boolean => { const fileKeys = files.map((file) => file.fileKey); const currentKey = cursorFile?.fileKey ?? null; @@ -200,14 +216,20 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput case "diff.nextFile": case "diff.previousFile": { if (files.length === 0) return false; - moveToFile(adjacentFileKey(fileKeys, currentKey, command === "diff.nextFile" ? 1 : -1)); + const delta = command === "diff.nextFile" ? 1 : -1; + moveToFile(fileKeyInView() ?? 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; - moveToFile(adjacentUnviewedFileKey(fileKeys, currentKey, delta, isKeyViewed)); + const inView = fileKeyInView(); + moveToFile( + inView !== null && !isKeyViewed(inView) + ? inView + : adjacentUnviewedFileKey(fileKeys, inView ?? currentKey, delta, isKeyViewed), + ); return true; } case "diff.toggleFileCollapsed": { diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index acf8ab4e1e17f..8439e6dab1ade 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -74,6 +74,7 @@ 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 or `Space` folds or unfolds the file. - `f` shows only the current file. `j`, `k`, and `n` switch which file it shows. From c3dda6b8be5c7e081f59bfbafa38bfc068d0feec Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:35:02 -0700 Subject: [PATCH 14/22] perf(web): diff review keys no longer redraw the whole diff Every cursor move re-rendered the Diff panel or PR Code tab, which rebuilt the viewer's item list and options. Pierre then reset its options, replaced its items and redrew every visible file synchronously, so each j/k took 30-40 ms and background updates redrew the diff too. The cursor now lives in a small store that only the outline, the file tree highlight and the line selection subscribe to. The styled viewer keeps its previous items and options while nothing in them changed, and the callbacks it receives are stable, so Pierre only redraws for real changes. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/DiffPanel.tsx | 533 +++++++++--------- .../components/diffs/AnnotatableCodeView.tsx | 13 +- .../components/diffs/DiffReviewControls.tsx | 4 +- .../components/diffs/StyledDiffCodeView.tsx | 12 +- .../components/diffs/keepUnchanged.test.ts | 40 ++ .../web/src/components/diffs/keepUnchanged.ts | 51 ++ .../useDiffReviewKeyboard.browser.test.tsx | 37 +- .../components/diffs/useDiffReviewKeyboard.ts | 134 ++++- .../pullRequest/PullRequestCodeTab.tsx | 145 +++-- 9 files changed, 616 insertions(+), 353 deletions(-) create mode 100644 apps/web/src/components/diffs/keepUnchanged.test.ts create mode 100644 apps/web/src/components/diffs/keepUnchanged.ts diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index c9117ca99e97a..9baffa36fec35 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -21,7 +21,7 @@ import { } from "lucide-react"; import { ChevronDown, ChevronRight, ChevronsDownUp, ChevronsUpDown } from "lucide"; 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 { useFilesystemReadAccess } from "~/state/filesystem"; import { useOpenInPreferredEditor } from "../editorPreferences"; @@ -77,7 +77,13 @@ import { DiffSingleFileToggle, DiffViewedToggle, } from "./diffs/DiffReviewControls"; -import { type DiffReviewFile, useDiffReviewKeyboard } from "./diffs/useDiffReviewKeyboard"; +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"; @@ -144,24 +150,36 @@ 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, collapsed, unavailable, - hasCursor, onToggle, }: { filePath: string; fileKey: string; collapsed: boolean; unavailable: boolean; - hasCursor: boolean; onToggle: (fileKey: string) => void; }) { return ( <> - {hasCursor ? : null} + fileKey === file.fileKey)), beginComment: (fileKey, range) => reviewRef.current?.beginComment(fileKey, range), }); + const onPostRender = useStableCallback(() => { + codeViewReveal.flush(); + review.flushScroll(); + }); const renderLoadingBoundary = () => review.singleFileKey === undefined && settledFileCount < renderableFiles.length ? ( - {!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.
- ) - ) : ( - <> -
- {isSelectedPatchTruncated && !lazySource && ( -

- This preview exceeds the size limit. Changes shown are incomplete. - {isGitSelection && selectedGitSource?.files ? " 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."} +
+ ) + ) : ( + <> +
+ {isSelectedPatchTruncated && !lazySource && ( +

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

- {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."} + )} + {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 controls own their clicks; bubbling a chevron click would toggle twice. - if ( - node instanceof HTMLButtonElement || - node instanceof HTMLAnchorElement || - node instanceof HTMLLabelElement - ) { + ) : !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"), ); - }} - {...(lazySource - ? { - unsafeCSSExtra: - "[data-additions-count], [data-deletions-count] { display: none; }", - } - : {})} - renderHeaderMetadata={(fileDiff) => { - const path = resolveFileDiffPath(fileDiff); - const stat = lazySource ? fileStats.get(path) : undefined; - return ( - <> - {stat ? ( - - ) : null} - {isFilePending(fileDiff) ? null : ( - { - setFileViewed(fileDiff, viewed); - setDiffFileCollapsed(getCachedFileEntry(fileDiff).fileKey, viewed); - }} - /> - )} - + const headerFilePath = header?.querySelector("[data-title]")?.textContent; + if (!headerFilePath) return; + const file = codeViewFiles.find( + (candidate) => candidate.filePath === headerFilePath, ); + if (file) toggleDiffFileCollapsed(file.fileKey); }} - onRevealSearchMatch={unfoldDiffFile} - renderHeaderPrefix={(fileDiff, fileKey, collapsed) => ( - - )} - 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: () => { - codeViewReveal.flush(); - review.flushScroll(); - }, - ...(currentLoadDiffFiles ? { loadDiffFiles } : {}), + 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, + ); }} - /> -
- {fileTreeOpen ? ( - - { + const path = resolveFileDiffPath(fileDiff); + const state = fileStates.get(path); + return ( + + ); + }} + {...(lazySource + ? { + unsafeCSSExtra: + "[data-additions-count], [data-deletions-count] { display: none; }", + } + : {})} + renderHeaderMetadata={(fileDiff) => { + const path = resolveFileDiffPath(fileDiff); + const stat = lazySource ? fileStats.get(path) : undefined; + return ( + <> + {stat ? ( + + ) : null} + {isFilePending(fileDiff) ? null : ( + { + setFileViewed(fileDiff, viewed); + setDiffFileCollapsed( + getCachedFileEntry(fileDiff).fileKey, + viewed, + ); + }} + /> + )} + + ); + }} + onRevealSearchMatch={unfoldDiffFile} + renderHeaderPrefix={(fileDiff, fileKey, collapsed) => ( + + )} + 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, + ...(currentLoadDiffFiles ? { loadDiffFiles } : {}), + }} /> - - ) : 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/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 83616f60587da..626e49871f560 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -12,6 +12,7 @@ import type { ScopedThreadRef } from "@t3tools/contracts"; import { 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, @@ -260,6 +261,14 @@ export function AnnotatableCodeView({ return true; }; useImperativeHandle(reviewRef, () => ({ beginComment })); + const onGutterUtilityClick = useStableCallback( + ( + range: SelectedLineRange | null, + { item }: { item: CodeViewItem }, + ) => { + if (range) beginComment(item.id, range); + }, + ); return ( @@ -285,9 +294,7 @@ export function AnnotatableCodeView({ ...options, enableGutterUtility: draft === null, enableLineSelection: draft === null, - onGutterUtilityClick: (range, { item }) => { - if (range) beginComment(item.id, range); - }, + onGutterUtilityClick, }} renderHeaderFilenameSuffix={(item) => item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null diff --git a/apps/web/src/components/diffs/DiffReviewControls.tsx b/apps/web/src/components/diffs/DiffReviewControls.tsx index 75300fcc44d68..5b310fbccc897 100644 --- a/apps/web/src/components/diffs/DiffReviewControls.tsx +++ b/apps/web/src/components/diffs/DiffReviewControls.tsx @@ -3,12 +3,14 @@ import { 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; /** Outlines the file header under the review cursor; render it in the header prefix. */ -export function DiffCursorOutline() { +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. ({ ...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 = ( - {...props} + {...viewProps} {...(viewerRef ? { ref: viewerRef } : {})} className={ className @@ -239,7 +247,7 @@ export function StyledDiffCodeView({ : "diff-render-surface [--code-background:var(--background)] outline-none" } options={{ - ...options, + ...keptOptions, unsafeCSS: unsafeCSSExtra ? `${DIFF_VIEW_UNSAFE_CSS}\n${unsafeCSSExtra}` : DIFF_VIEW_UNSAFE_CSS, 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/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 077234e90763b..9f6e866ff85be 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -3,7 +3,11 @@ import { expect, it, vi } from "vite-plus/test"; import { userEvent } from "vite-plus/test/browser"; import { render } from "../../../test/render"; -import { useDiffReviewKeyboard } from "./useDiffReviewKeyboard"; +import { + DiffReviewCursorContext, + useDiffReviewCursorFilePath, + useDiffReviewKeyboard, +} from "./useDiffReviewKeyboard"; vi.mock("~/state/server", async () => { const { Atom } = await import("effect/reactivity"); @@ -18,8 +22,20 @@ const file = { rendered: true, }; -function Panel(props: { folds: string[]; clicks: string[]; reveals?: string[]; hidden?: boolean }) { - const { rootRef } = useDiffReviewKeyboard({ +function CursorFile() { + const filePath = useDiffReviewCursorFilePath(); + return filePath === null ? null :

Cursor on {filePath}

; +} + +function Panel(props: { + folds: string[]; + clicks: string[]; + reveals?: string[]; + renders?: string[]; + hidden?: boolean; +}) { + props.renders?.push("panel"); + const { rootRef, cursorStore } = useDiffReviewKeyboard({ scopeKey: "scope", files: [file], diffStyle: "unified", @@ -44,6 +60,9 @@ function Panel(props: { folds: string[]; clicks: string[]; reveals?: string[]; h + + +
); } @@ -105,3 +124,15 @@ it("takes the keys from anywhere in the panel showing the diff", async () => { 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); +}); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index cef999d0fb577..922ce9e9b9d2d 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -9,7 +9,7 @@ import type { import type { CodeViewHandle } from "@pierre/diffs/react"; import type { KeybindingCommand } from "@t3tools/contracts"; import * as Schema from "effect/Schema"; -import { useEffect, useRef, useState } from "react"; +import { createContext, use, useEffect, useRef, useState, useSyncExternalStore } from "react"; import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useStableCallback } from "~/hooks/useStableCallback"; @@ -73,6 +73,65 @@ 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; @@ -105,10 +164,14 @@ interface DiffReviewKeyboardInput { */ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput) { const { scopeKey, files } = input; - const [cursorState, setCursorState] = useState<{ - readonly scopeKey: string | null; - readonly cursor: DiffReviewCursor | null; - }>({ scopeKey, cursor: null }); + 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( @@ -121,25 +184,32 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput // Chrome marks a clicked control :focus-visible on the next key press, so record how focus came. const tabbedToRef = useRef(null); - const storedCursor = cursorState.scopeKey === scopeKey ? cursorState.cursor : null; - const cursorFile = storedCursor - ? (files.find((file) => file.fileKey === storedCursor.fileKey) ?? - files.find((file) => file.filePath === storedCursor.filePath) ?? - null) - : null; - const singleFileKey = singleFile ? (cursorFile ?? files[0])?.fileKey : undefined; + // 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); - const cursor: DiffReviewCursor | null = - storedCursor === null || cursorFile === null - ? null - : storedCursor.mode !== "file" && (!cursorFile.rendered || isCollapsed(cursorFile.fileKey)) - ? { mode: "file", fileKey: cursorFile.fileKey, filePath: cursorFile.filePath } - : { ...storedCursor, fileKey: cursorFile.fileKey }; - const range = cursor ? cursorSelectedRange(cursor) : null; - const selectedLines: CodeViewLineSelection | null = - cursor && range ? { id: cursor.fileKey, range } : null; + // 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) => setCursorState({ scopeKey, cursor: next }); + const setCursor = (next: DiffReviewCursor | null) => cursorStore.set(next); const fileCursor = (file: { fileKey: string; filePath: string }): DiffReviewCursor => ({ mode: "file", fileKey: file.fileKey, @@ -191,7 +261,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput }; // After the reader scrolls away from the cursor, the file keys start from what is on screen. - const fileKeyInView = () => { + const fileKeyInView = (cursorKey: string | null) => { const viewer = input.getViewer()?.getInstance(); if (!viewer) return null; const top = viewer.getScrollTop(); @@ -200,12 +270,14 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput const fileTop = viewer.getTopForItem(fileKey); return fileTop === undefined ? [] : [{ fileKey, top: fileTop }]; }), - cursorFile?.fileKey ?? null, + cursorKey, { top, bottom: top + viewer.getHeight(), contentEnd: viewer.getScrollHeight() }, ); }; 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) => { @@ -217,14 +289,14 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput case "diff.previousFile": { if (files.length === 0) return false; const delta = command === "diff.nextFile" ? 1 : -1; - moveToFile(fileKeyInView() ?? adjacentFileKey(fileKeys, currentKey, delta)); + 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(); + const inView = fileKeyInView(currentKey); moveToFile( inView !== null && !isKeyViewed(inView) ? inView @@ -343,7 +415,10 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput } return handled; }); - const isLineMode = useStableCallback(() => cursor !== null && cursor.mode !== "file"); + 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(() => { @@ -390,10 +465,9 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput shortcutLabelForCommand(keybindings, command, { context: { diffFocus: true, diffLineMode } }); return { - cursor, + cursorStore, singleFile, singleFileKey, - selectedLines, rootRef, flushScroll, shortcutLabels: { @@ -409,6 +483,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput }, onSelectedLinesChange: (selection: CodeViewLineSelection | null) => { if (selection === null) { + const { cursor } = readCursor(); if (cursor && cursor.mode !== "file") setCursor(fileCursor(cursor)); return; } @@ -417,6 +492,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput }, // 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", diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 2643665934824..586efd681c6b6 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -2,7 +2,12 @@ import { capitalizeTerm, sourceControlClients, } from "@t3tools/client-runtime/source-control-clients"; -import type { CodeViewItem, DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs"; +import type { + CodeViewItem, + DiffLineAnnotation, + FileDiffContentsLoader, + SelectedLineRange, +} from "@pierre/diffs"; import type { CodeViewAnnotationArgs, CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; import type { EnvironmentId, @@ -39,11 +44,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"; @@ -88,7 +95,11 @@ import { DiffSingleFileToggle, DiffViewedToggle, } from "../diffs/DiffReviewControls"; -import { useDiffReviewKeyboard } from "../diffs/useDiffReviewKeyboard"; +import { + DiffReviewCursorContext, + useDiffReviewKeyboard, + useDiffReviewSelectedLines, +} from "../diffs/useDiffReviewKeyboard"; import { useHideTestFiles } from "../diffs/useHideTestFiles"; import { diffFileTreeEntries, orderDiffFilesLikeTree } from "../diffs/diffFileTree.logic"; import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; @@ -189,6 +200,12 @@ interface FileViewedToggleState { 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. const FileViewedToggleContext = createContext(null); @@ -454,12 +471,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 hostReview = detail.capabilities.review; const review = { inlineComment: hostReview.inlineComment && detail.viewerPermissions.comment, @@ -771,34 +791,33 @@ 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) 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); @@ -851,7 +870,9 @@ function PullRequestCodeTab({ const collapsed = item.collapsed === true; return ( <> - {diffReview.cursor?.fileKey === item.id ? : null} + {item.type === "diff" ? ( + + ) : null}
{fileTreeOpen ? ( From 326b719657a23c5acc60c2956f6d32e85409e063 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:18:50 -0700 Subject: [PATCH 15/22] fix(web): closing the file in single-file view returns to the full list Single-file view keeps its one file open, so its collapse arrow, a title click, Enter and "Collapse all" did nothing there. The view also persists across reloads, so it looked like the diff list had broken. Collapsing the shown file, or using expand/collapse all, now leaves single-file view and returns to the full list. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/DiffPanel.tsx | 16 +++------------- .../useDiffReviewKeyboard.browser.test.tsx | 19 ++++++++++++++++++- .../components/diffs/useDiffReviewKeyboard.ts | 11 ++++++++++- .../pullRequest/PullRequestCodeTab.tsx | 6 +++--- 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 9baffa36fec35..0924d5d20a609 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -828,17 +828,6 @@ export default function DiffPanel({ }, }); }; - const toggleDiffFileCollapsed = useStableCallback((fileKey: string) => { - updateCollapsedDiffFiles((current) => { - const next = new Set(current); - if (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) => { @@ -850,6 +839,7 @@ export default function DiffPanel({ }); const toggleDiffFileCollapse = () => { + review.showAllFiles(); setCodeViewRevision((current) => current + 1); updateCollapsedDiffFiles((current) => toggleAllDiffFiles(shownFileKeys, current)); }; @@ -1487,7 +1477,7 @@ export default function DiffPanel({ const file = codeViewFiles.find( (candidate) => candidate.filePath === headerFilePath, ); - if (file) toggleDiffFileCollapsed(file.fileKey); + if (file) review.toggleFileCollapsed(file.fileKey); }} onContextMenuCapture={(event) => { const composedPath = event.nativeEvent.composedPath?.() ?? []; @@ -1578,7 +1568,7 @@ export default function DiffPanel({ fileKey={fileKey} collapsed={collapsed} unavailable={isFilePending(fileDiff)} - onToggle={toggleDiffFileCollapsed} + onToggle={review.toggleFileCollapsed} /> )} options={{ diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 9f6e866ff85be..fe1918864e360 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -35,7 +35,7 @@ function Panel(props: { hidden?: boolean; }) { props.renders?.push("panel"); - const { rootRef, cursorStore } = useDiffReviewKeyboard({ + const { rootRef, cursorStore, singleFile, toggleFileCollapsed } = useDiffReviewKeyboard({ scopeKey: "scope", files: [file], diffStyle: "unified", @@ -60,6 +60,10 @@ function Panel(props: { + + {singleFile ?

One file at a time

: null} @@ -136,3 +140,16 @@ it("moves the cursor without redrawing the panel", async () => { 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"]); +}); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index 922ce9e9b9d2d..bd8df435cf566 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -275,6 +275,12 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput ); }; + 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; @@ -306,7 +312,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput } case "diff.toggleFileCollapsed": { if (!cursorFile) return false; - input.setCollapsed(cursorFile.fileKey, !isCollapsed(cursorFile.fileKey)); + toggleFileCollapsed(cursorFile.fileKey); return true; } case "diff.toggleFileViewed": { @@ -476,6 +482,9 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput 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); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 586efd681c6b6..47330ca36cb4a 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -671,7 +671,6 @@ function PullRequestCodeTab({ setToggledFiles((current) => toggleFileDiffFoldForViewed(fileKey, collapsed, effectiveFoldOverride, current), ); - const toggleFile = (fileKey: string) => setCollapsed(fileKey, !isCollapsed(fileKey)); const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { const range = getFileViewedRange( annotatedFiles, @@ -782,6 +781,7 @@ function PullRequestCodeTab({ }; const toggleAllFiles = () => { + diffReview.showAllFiles(); setFoldOverride(areAllDiffFilesCollapsed(fileKeys, collapsedFileKeys) ? "expanded" : "folded"); setToggledFiles(new Set()); }; @@ -881,7 +881,7 @@ function PullRequestCodeTab({ className="mr-1" onClick={(event) => { event.stopPropagation(); - toggleFile(item.id); + diffReview.toggleFileCollapsed(item.id); }} > @@ -1536,7 +1536,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; } } From b1d5e1dcda80ab543ca2fb46961d582bb34b2bf6 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Mon, 2 Nov 2026 14:55:41 -0700 Subject: [PATCH 16/22] feat(web): diff review marks a finished review and steps files without jumping - The viewed count reads "All N" with a check once every file is viewed. - Enter alone folds a file; Space no longer does. - Shift+J and Shift+K reveal 20 lines at a time, keep the cursor on screen, and load the full file when line mode starts so the first press works. - Moving between files scrolls only as far as needed while the file fits on screen, instead of lining every file up at the top. - In line mode, k on the first line returns to the file and j on the last line moves on to the next file. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/DiffPanel.tsx | 14 ++-- .../components/diffs/DiffReviewControls.tsx | 19 ++++- .../components/diffs/diffReviewCursor.test.ts | 14 ++++ .../src/components/diffs/diffReviewCursor.ts | 16 ++++ .../components/diffs/useCodeViewFileReveal.ts | 11 +-- .../useDiffReviewKeyboard.browser.test.tsx | 35 ++++++-- .../components/diffs/useDiffReviewKeyboard.ts | 79 ++++++++++++++----- .../pullRequest/PullRequestCodeTab.tsx | 8 +- apps/web/src/keybindings.test.ts | 3 +- docs/user/keybindings.md | 9 ++- packages/shared/src/keybindings.ts | 1 - 11 files changed, 162 insertions(+), 47 deletions(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 0924d5d20a609..047ac60f6d2a5 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -75,6 +75,7 @@ import { import { DiffCursorOutline, DiffSingleFileToggle, + DiffViewedCount, DiffViewedToggle, } from "./diffs/DiffReviewControls"; import { @@ -721,13 +722,13 @@ export default function DiffPanel({ viewerHandleRef.current = handle; codeViewReveal.attach(handle); }); - const revealFileAt = (index: number) => { + const revealFileAt = (index: number, align?: "start" | "nearest") => { const file = renderableFileEntries[index]; if (!file) return; if (lazySource && index >= settledFileCount) { requestFile(index); } - codeViewReveal.request(file.fileKey); + codeViewReveal.request(file.fileKey, align); }; const reviewRef = useRef(null); const { rootRef: reviewRootRef, ...review } = useDiffReviewKeyboard({ @@ -741,8 +742,11 @@ export default function DiffPanel({ setCollapsed: setDiffFileCollapsed, isViewed: (file) => isFileViewed(file.fileDiff), setViewed: (file, viewed) => setFileViewed(file.fileDiff, viewed), - revealFile: (file) => - revealFileAt(renderableFileEntries.findIndex(({ fileKey }) => fileKey === file.fileKey)), + revealFile: (file, align) => + revealFileAt( + renderableFileEntries.findIndex(({ fileKey }) => fileKey === file.fileKey), + align, + ), beginComment: (fileKey, range) => reviewRef.current?.beginComment(fileKey, range), }); const onPostRender = useStableCallback(() => { @@ -1188,7 +1192,7 @@ export default function DiffPanel({ > {shownFileEntries.length > 0 ? ( - {viewedFileCount} / {shownFileEntries.length} viewed + viewed ) : null} {codeViewFiles.length > 0 || (isGitSelection && selectedGitSource?.files?.length) ? ( diff --git a/apps/web/src/components/diffs/DiffReviewControls.tsx b/apps/web/src/components/diffs/DiffReviewControls.tsx index 5b310fbccc897..0bec938b90055 100644 --- a/apps/web/src/components/diffs/DiffReviewControls.tsx +++ b/apps/web/src/components/diffs/DiffReviewControls.tsx @@ -1,4 +1,4 @@ -import { FileIcon } from "lucide-react"; +import { CheckIcon, FileIcon } from "lucide-react"; import { Checkbox } from "../ui/checkbox"; import { Toggle } from "../ui/toggle-group"; @@ -8,6 +8,23 @@ 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; diff --git a/apps/web/src/components/diffs/diffReviewCursor.test.ts b/apps/web/src/components/diffs/diffReviewCursor.test.ts index e9a461125c037..ad2a561f79366 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.test.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.test.ts @@ -7,6 +7,7 @@ import { cursorFromSelectedRange, cursorSelectedRange, type DiffRow, + fileScrollAlign, fileToStartFrom, nearestGapExpansion, stepRow, @@ -183,4 +184,17 @@ describe("file navigation", () => { 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 index 3f0016ceba790..714baccc93b8f 100644 --- a/apps/web/src/components/diffs/diffReviewCursor.ts +++ b/apps/web/src/components/diffs/diffReviewCursor.ts @@ -176,6 +176,22 @@ export function fileToStartFrom( 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, diff --git a/apps/web/src/components/diffs/useCodeViewFileReveal.ts b/apps/web/src/components/diffs/useCodeViewFileReveal.ts index f0779262dd1ac..e44afb333769a 100644 --- a/apps/web/src/components/diffs/useCodeViewFileReveal.ts +++ b/apps/web/src/components/diffs/useCodeViewFileReveal.ts @@ -15,11 +15,11 @@ interface FileRevealHandle { */ export function createCodeViewFileReveal() { let viewer: FileRevealHandle | null = null; - let pending: string | null = null; + let pending: { fileKey: string; align: "start" | "nearest" } | null = null; const flush = () => { if (pending === null || viewer === null) return; - if (!viewer.getInstance() || viewer.getItem(pending) === undefined) return; - viewer.scrollTo({ type: "item", id: pending, align: "start" }); + if (!viewer.getInstance() || viewer.getItem(pending.fileKey) === undefined) return; + viewer.scrollTo({ type: "item", id: pending.fileKey, align: pending.align }); pending = null; }; return { @@ -28,8 +28,9 @@ export function createCodeViewFileReveal() { viewer = next; flush(); }, - request(fileKey: string) { - pending = fileKey; + /** `nearest` leaves a file already on screen in place. */ + request(fileKey: string, align: "start" | "nearest" = "start") { + pending = { fileKey, align }; flush(); }, flush, diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index fe1918864e360..991ffce3d2c47 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -7,6 +7,7 @@ import { DiffReviewCursorContext, useDiffReviewCursorFilePath, useDiffReviewKeyboard, + useDiffReviewSelectedLines, } from "./useDiffReviewKeyboard"; vi.mock("~/state/server", async () => { @@ -21,10 +22,12 @@ const file = { 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(); - return filePath === null ? null :

Cursor on {filePath}

; + const onLines = useDiffReviewSelectedLines() !== null; + return filePath === null ? null :

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

; } function Panel(props: { @@ -33,11 +36,12 @@ function Panel(props: { reveals?: string[]; renders?: string[]; hidden?: boolean; + twoFiles?: boolean; }) { props.renders?.push("panel"); const { rootRef, cursorStore, singleFile, toggleFileCollapsed } = useDiffReviewKeyboard({ scopeKey: "scope", - files: [file], + files: props.twoFiles ? [file, otherFile] : [file], diffStyle: "unified", canLoadFullFiles: false, autoAdvanceOnViewed: false, @@ -83,23 +87,23 @@ it("takes the keys from a clicked control and drops its focus ring", async () => await userEvent.keyboard("j"); expect(document.activeElement).toBe(screen.getByTestId("panel").element()); await userEvent.click(screen.getByRole("button", { name: "Changes" })); - await userEvent.keyboard("{Enter} "); + await userEvent.keyboard("{Enter}"); - expect(folds).toEqual(["a", "a"]); + expect(folds).toEqual(["a"]); expect(clicks).toEqual(["Changes", "Changes"]); }); -it("leaves only Enter and Space to a control reached with Tab", async () => { +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} "); + await userEvent.keyboard("{Tab}{Enter}"); expect(folds).toEqual([]); - expect(clicks).toEqual(["Changes", "Changes"]); + expect(clicks).toEqual(["Changes"]); await userEvent.keyboard("j"); expect(document.activeElement).toBe(screen.getByTestId("panel").element()); }); @@ -153,3 +157,20 @@ it("returns to the full list when the single shown file is closed", async () => 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 index bd8df435cf566..c9512a221e894 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -25,6 +25,7 @@ import { cursorSelectedRange, type DiffReviewCursor, type DiffRowRef, + fileScrollAlign, fileToStartFrom, nearestGapExpansion, stepRow, @@ -65,6 +66,8 @@ const REPEATABLE_COMMANDS: ReadonlySet = new Set([ 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"]'; @@ -153,8 +156,8 @@ interface DiffReviewKeyboardInput { readonly setCollapsed: (fileKey: string, collapsed: boolean) => void; readonly isViewed: (file: DiffReviewFile) => boolean; readonly setViewed: (file: DiffReviewFile, viewed: boolean) => void; - /** Loads the file if needed and scrolls its header into view. */ - readonly revealFile: (file: DiffReviewFile) => void; + /** 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; } @@ -242,37 +245,53 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput pendingScrollRef.current = null; viewer.scrollTo(target); }); - const scrollToRow = (file: DiffReviewFile, row: DiffRowRef) => { + const queueScrollToRow = ( + file: DiffReviewFile, + row: DiffRowRef, + align: "nearest" | "center" = "nearest", + ) => { pendingScrollRef.current = { type: "line", id: file.fileKey, lineNumber: row.lineNumber, side: row.side, - align: "nearest", + 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)); - input.revealFile(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 viewer = input.getViewer()?.getInstance(); - if (!viewer) return null; - const top = viewer.getScrollTop(); - return fileToStartFrom( - files.flatMap(({ fileKey }) => { - const fileTop = viewer.getTopForItem(fileKey); - return fileTop === undefined ? [] : [{ fileKey, top: fileTop }]; - }), - cursorKey, - { top, bottom: top + viewer.getHeight(), contentEnd: viewer.getScrollHeight() }, - ); + const placement = placeFiles(); + return placement ? fileToStartFrom(placement.placed, cursorKey, placement.view) : null; }; const toggleFileCollapsed = useStableCallback((fileKey: string) => { @@ -348,6 +367,9 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput 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. + if (canExpand(file)) void fileDiffInstance(file.fileKey)?.prepareForEditing(); setCursor({ mode: "line", fileKey: file.fileKey, filePath: file.filePath, row: firstRow }); scrollToRow(file, firstRow); return true; @@ -357,8 +379,16 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput if (!cursor || !cursorFile || cursor.mode === "file") return false; const delta = command === "diff.nextLine" ? 1 : -1; const rows = rowsFor(cursorFile); - const next = stepRow(rows, cursor.mode === "line" ? cursor.row : cursor.head, delta); + 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); @@ -369,13 +399,20 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput 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), - cursor.mode === "line" ? cursor.row : cursor.head, + row, command === "diff.expandBelow" ? "below" : "above", ); - if (gap) instance.expandHunk(gap.hunkIndex, gap.direction); + 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": { @@ -450,9 +487,9 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput capture: true, when: (event) => { if (!rootRef.current || !diffPanelHasFocus(rootRef.current)) return false; - // A control reached from the keyboard keeps Enter and Space; a clicked one hands them over. + // A control reached from the keyboard keeps Enter; a clicked one hands it over. return !( - (event.key === "Enter" || event.key === " ") && + event.key === "Enter" && isControlFocused() && document.activeElement === tabbedToRef.current ); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 47330ca36cb4a..fb6cd87d29bf1 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -93,6 +93,7 @@ import { useCodeViewFileReveal } from "../diffs/useCodeViewFileReveal"; import { DiffCursorOutline, DiffSingleFileToggle, + DiffViewedCount, DiffViewedToggle, } from "../diffs/DiffReviewControls"; import { @@ -710,7 +711,7 @@ function PullRequestCodeTab({ setViewed: (file, viewed) => { if (filesViewedEnabled) setFileViewed(file.fileKey, viewed, false); }, - revealFile: (file) => codeViewReveal.request(file.fileKey), + revealFile: (file, align) => codeViewReveal.request(file.fileKey, align), beginComment: (fileKey, range) => { const item = items.find((candidate) => candidate.id === fileKey); if (item) beginComment(range, { item }); @@ -1135,7 +1136,10 @@ function PullRequestCodeTab({ {filesViewed.enabled && files.length > 0 ? ( - {filePaths.filter(isFileViewed).length} / {files.length} + {viewedFilesStore === "environment" ? `viewed in ${APP_BASE_NAME}` : "viewed"} diff --git a/apps/web/src/keybindings.test.ts b/apps/web/src/keybindings.test.ts index 3b2da43241f45..608f798f3fa81 100644 --- a/apps/web/src/keybindings.test.ts +++ b/apps/web/src/keybindings.test.ts @@ -938,8 +938,7 @@ describe("Diff review shortcuts", () => { ], [event({ key: "v" }), { diffFocus: true, diffLineMode: true }, "diff.toggleFileViewed"], [event({ key: "c" }), { diffFocus: true, diffLineMode: true }, "diff.comment"], - [event({ key: " ", code: "Space" }), { diffFocus: true, diffLineMode: true }, null], - [event({ key: " ", code: "Space" }), { diffFocus: true }, "diff.toggleFileCollapsed"], + [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], diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 8439e6dab1ade..9a59aa4999512 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -76,12 +76,14 @@ On files: - `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 or `Space` folds or unfolds the file. +- 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. +- `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. @@ -93,7 +95,8 @@ In the Diff panel, viewed marks belong to this device and thread, and a file tha 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**. +mark, turn on **Settings → Diffs → Auto-advance after marking viewed**. Once every +file is viewed, the count above the diff reads **All 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 diff --git a/packages/shared/src/keybindings.ts b/packages/shared/src/keybindings.ts index cb66804f58a87..9ee5994386db9 100644 --- a/packages/shared/src/keybindings.ts +++ b/packages/shared/src/keybindings.ts @@ -169,7 +169,6 @@ export const DEFAULT_KEYBINDINGS: ReadonlyArray = [ { 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: "space", 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" }, From a7bed0f08daa01ee3e147cceb32c37bdff718019 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:26:20 -0700 Subject: [PATCH 17/22] fix(web): v leaves files alone where they can't be marked viewed In a pull request without viewed marks, v folded the file and skipped ahead without recording anything, and a second v didn't unfold it. The Code tab now passes no setViewed there, so the key is left alone. The keybindings guide also quotes the completed count as the UI shows it. Co-Authored-By: Claude Opus 5.5 --- .../diffs/useDiffReviewKeyboard.browser.test.tsx | 13 ++++++++++++- .../src/components/diffs/useDiffReviewKeyboard.ts | 5 +++-- .../components/pullRequest/PullRequestCodeTab.tsx | 6 +++--- docs/user/keybindings.md | 2 +- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 991ffce3d2c47..57ef87bc7bec3 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -37,6 +37,7 @@ function Panel(props: { renders?: string[]; hidden?: boolean; twoFiles?: boolean; + viewedUnsupported?: boolean; }) { props.renders?.push("panel"); const { rootRef, cursorStore, singleFile, toggleFileCollapsed } = useDiffReviewKeyboard({ @@ -49,7 +50,7 @@ function Panel(props: { isCollapsed: () => false, setCollapsed: (fileKey) => void props.folds.push(fileKey), isViewed: () => false, - setViewed: () => {}, + setViewed: props.viewedUnsupported ? null : () => {}, revealFile: (revealed) => void props.reveals?.push(revealed.fileKey), beginComment: () => {}, }); @@ -93,6 +94,16 @@ it("takes the keys from a clicked control and drops its focus ring", async () => 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([]); +}); + it("leaves Enter to a control reached with Tab", async () => { const folds: string[] = []; const clicks: string[] = []; diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index c9512a221e894..c1b27474e3e63 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -155,7 +155,8 @@ interface DiffReviewKeyboardInput { readonly isCollapsed: (fileKey: string) => boolean; readonly setCollapsed: (fileKey: string, collapsed: boolean) => void; readonly isViewed: (file: DiffReviewFile) => boolean; - readonly setViewed: (file: DiffReviewFile, viewed: boolean) => void; + /** 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; @@ -335,7 +336,7 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput return true; } case "diff.toggleFileViewed": { - if (!cursorFile) return false; + if (!cursorFile || !input.setViewed) return false; if (!cursorFile.rendered) return true; const viewed = !input.isViewed(cursorFile); input.setViewed(cursorFile, viewed); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index fb6cd87d29bf1..9bfc109e5177d 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -708,9 +708,9 @@ function PullRequestCodeTab({ isCollapsed, setCollapsed, isViewed: (file) => filesViewedEnabled && isFileViewed(file.filePath), - setViewed: (file, viewed) => { - if (filesViewedEnabled) setFileViewed(file.fileKey, viewed, false); - }, + setViewed: filesViewedEnabled + ? (file, viewed) => setFileViewed(file.fileKey, viewed, false) + : null, revealFile: (file, align) => codeViewReveal.request(file.fileKey, align), beginComment: (fileKey, range) => { const item = items.find((candidate) => candidate.id === fileKey); diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 9a59aa4999512..dc42f76ab3786 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -96,7 +96,7 @@ 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 **All viewed**. +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 From 99ea4814edc34f2b0c68c6642326a0888faf353d Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Tue, 3 Nov 2026 06:17:53 +0000 Subject: [PATCH 18/22] fix(web): retain hydrated context during keyboard diff review --- .../useDiffReviewKeyboard.browser.test.tsx | 33 ++++++++++++++++++- .../components/diffs/useDiffReviewKeyboard.ts | 8 +++-- patches/@pierre%2Fdiffs@1.6.1.patch | 9 +++++ pnpm-lock.yaml | 10 +++--- 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 57ef87bc7bec3..3415aa223a1f1 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -1,8 +1,11 @@ import { parseDiffFromFile } from "@pierre/diffs"; +// oxlint-disable-next-line eslint/no-restricted-imports -- exercises hydration in the patched dependency without app theme services +import { CodeView, type CodeViewHandle } from "@pierre/diffs/react"; +import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; import { expect, it, vi } from "vite-plus/test"; import { userEvent } from "vite-plus/test/browser"; -import { render } from "../../../test/render"; +import { act, render } from "../../../test/render"; import { DiffReviewCursorContext, useDiffReviewCursorFilePath, @@ -185,3 +188,31 @@ it("leaves a file's lines past either end", async () => { await expect.element(screen.getByText("Cursor on b.ts")).toBeInTheDocument(); expect(folds).toEqual(foldsBefore); }); + +it("expands unchanged context after an equivalent partial diff refreshes", async () => { + const patch = "--- a/review.txt\n+++ b/review.txt\n@@ -48 +48 @@\n-context 48\n+changed 48\n"; + const before = Array.from({ length: 80 }, (_, index) => `context ${index + 1}\n`).join(""); + const viewer = { current: null as CodeViewHandle | null }; + const options = { + diffStyle: "unified" as const, + loadDiffFiles: async () => ({ + oldFile: { name: "review.txt", contents: before }, + newFile: { name: "review.txt", contents: before.replace("context 48", "changed 48") }, + }), + }; + const items = (version: number) => [ + { + id: "review", + type: "diff" as const, + version, + fileDiff: parsePatchFiles(patch, "refresh")[0]!.files[0]!, + }, + ]; + await using screen = await render(); + await vi.waitFor(() => expect(viewer.current?.getInstance()?.getRenderedItems()).toHaveLength(1)); + await screen.rerender(); + const rendered = viewer.current!.getInstance()!.getRenderedItems()[0]!; + if (rendered.type !== "diff") throw new Error("Expected a diff"); + await act(() => rendered.instance.expandHunk(0, "down", 20)); + await expect.element(screen.getByText("context 47", { exact: true })).toBeVisible(); +}); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index c1b27474e3e63..b902ae30bf523 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -228,12 +228,14 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput .find((candidate) => candidate.id === fileKey); return item?.type === "diff" ? item.instance : undefined; }; - const rowsFor = (file: DiffReviewFile) => - visibleDiffRows( - file.fileDiff, + 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)?.getExpandedHunks() ?? NO_EXPANSIONS, ); + }; const canExpand = (file: DiffReviewFile) => !file.fileDiff.isPartial || input.canLoadFullFiles; const flushScroll = useStableCallback(() => { diff --git a/patches/@pierre%2Fdiffs@1.6.1.patch b/patches/@pierre%2Fdiffs@1.6.1.patch index 12cee93921e3b..46c387ca21da4 100644 --- a/patches/@pierre%2Fdiffs@1.6.1.patch +++ b/patches/@pierre%2Fdiffs@1.6.1.patch @@ -854,6 +854,15 @@ diff --git a/dist/components/VirtualizedFileDiff.js b/dist/components/Virtualize index eb34dd4a774cb06add3280fe6abd5f0dc292505e..821bb0ab114f52ec41eab54505a1eb08e511f47a 100644 --- a/dist/components/VirtualizedFileDiff.js +++ b/dist/components/VirtualizedFileDiff.js +@@ -227,7 +227,7 @@ var VirtualizedFileDiff = class extends FileDiff { + const { pendingExpansions, pendingHydratedDiff, options: { collapsed = false } } = this; + if (pendingHydratedDiff != null) { + this.pendingHydratedDiff = void 0; +- if (pendingHydratedDiff.expectedDiff === fileDiff) { ++ if (areDiffTargetsEqual(pendingHydratedDiff.expectedDiff, fileDiff)) { + Object.assign(fileDiff, pendingHydratedDiff.nextDiff); + this.setHydratedState(pendingHydratedDiff.files); + this.installHydratedSessionDiff(fileDiff); @@ -551,7 +551,7 @@ var VirtualizedFileDiff = class extends FileDiff { rerender() { if (!this.enabled || this.getInputDiff() == null && this.additionFile == null && this.deletionFile == null) return; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1299a6be36be5..5d9b21535fa10 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -367,7 +367,7 @@ patchedDependencies: '@ff-labs/fff-node@0.11.0': cc71fcec015f4d39bc96ae2e2bf6b0ca678124c722c66a7841efce9d34b5983c '@legendapp/list@3.3.5': 6cbdacd8c380d9a5afb9f73d4198dc2cc44be6ef28a955b1183c781469307896 '@muse-code/sdk@1.4.6': 7b495b607fc5d76b444c7c4d58821e839726a30e6f3c53564f0d59f2d5b42101 - '@pierre/diffs@1.6.1': 0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08 + '@pierre/diffs@1.6.1': af9bc1a1284bc5d68e46947ffaa92809beaa828013d74ba28dadbd1222e75cf6 '@react-native-ai/apple@0.12.0': 2d09870c2848d185cb05b53ed823a46e12dba519324d8dd8e584e28731990f9d '@react-native-menu/menu@2.0.0': 910ec1bf22aa85a96fcf12299cd823bc30f702dd9b2d77b564d10b9ed709f7f6 '@react-navigation/native-stack@7.21.0': 990d575444c4af2ce8218b3b7a8280fe244c03abbdb32b822a2e862f791dbf42 @@ -557,7 +557,7 @@ importers: version: 1.9.1 '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=af9bc1a1284bc5d68e46947ffaa92809beaa828013d74ba28dadbd1222e75cf6)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@pierre/highlights': specifier: 'catalog:' version: 0.2.0 @@ -1063,7 +1063,7 @@ importers: version: 1.8.0 '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=af9bc1a1284bc5d68e46947ffaa92809beaa828013d74ba28dadbd1222e75cf6)(@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) @@ -1485,7 +1485,7 @@ importers: dependencies: '@pierre/diffs': specifier: 1.6.1 - version: 1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0) + version: 1.6.1(patch_hash=af9bc1a1284bc5d68e46947ffaa92809beaa828013d74ba28dadbd1222e75cf6)(@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 @@ -15575,7 +15575,7 @@ snapshots: tslib: 2.8.1 webcrypto-core: 1.9.2 - '@pierre/diffs@1.6.1(patch_hash=0c62e79cffd522f80634317767793090fefa59a28d2ed7d79d7afbe08e143f08)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': + '@pierre/diffs@1.6.1(patch_hash=af9bc1a1284bc5d68e46947ffaa92809beaa828013d74ba28dadbd1222e75cf6)(@shikijs/themes@4.2.0)(react-dom@19.3.0(react@19.3.0))(react@19.3.0)': dependencies: '@pierre/highlights': 0.2.0 '@pierre/theme': 2.0.0 From c76e4e4264dd9dddbf2533e332aafc0444b4d92f Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Tue, 3 Nov 2026 09:31:04 -0700 Subject: [PATCH 19/22] refactor(web): keep the Code tab's viewer ref stable attachViewer leaves the component as the viewer's ref, so it now uses useStableCallback like the Diff panel's. The test for v without viewed marks also checks that auto-advance leaves the cursor in place. Co-Authored-By: Claude Opus 5.5 --- .../diffs/useDiffReviewKeyboard.browser.test.tsx | 9 +++++++-- .../src/components/pullRequest/PullRequestCodeTab.tsx | 10 ++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 3415aa223a1f1..861d5a281e942 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -41,6 +41,7 @@ function Panel(props: { hidden?: boolean; twoFiles?: boolean; viewedUnsupported?: boolean; + autoAdvance?: boolean; }) { props.renders?.push("panel"); const { rootRef, cursorStore, singleFile, toggleFileCollapsed } = useDiffReviewKeyboard({ @@ -48,7 +49,7 @@ function Panel(props: { files: props.twoFiles ? [file, otherFile] : [file], diffStyle: "unified", canLoadFullFiles: false, - autoAdvanceOnViewed: false, + autoAdvanceOnViewed: props.autoAdvance ?? false, getViewer: () => null, isCollapsed: () => false, setCollapsed: (fileKey) => void props.folds.push(fileKey), @@ -99,12 +100,16 @@ it("takes the keys from a clicked control and drops its focus ring", async () => it("leaves v alone where files cannot be marked viewed", async () => { const folds: string[] = []; - await using screen = await render(, options); + 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 () => { diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 9bfc109e5177d..778f59bb10a8c 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -689,10 +689,12 @@ function PullRequestCodeTab({ ); }; const viewerHandleRef = useRef | null>(null); - const attachViewer = (handle: CodeViewHandle | null) => { - viewerHandleRef.current = handle; - codeViewReveal.attach(handle); - }; + const attachViewer = useStableCallback( + (handle: CodeViewHandle | null) => { + viewerHandleRef.current = handle; + codeViewReveal.attach(handle); + }, + ); const { rootRef: reviewRootRef, ...diffReview } = useDiffReviewKeyboard({ scopeKey, files: annotatedFiles.map(({ fileKey, path, fileDiff }) => ({ From 2930358d94ea47f3496fa919e11f14d81fc9a6cf Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:17:34 -0700 Subject: [PATCH 20/22] fix(web): review comments keep their line and focus In the Code tab, pressing the comment key while a draft was open moved that draft to the cursor's line. It now leaves the open draft alone, as the Diff panel already did. When a refresh drops the file of a draft in the Diff panel, focus now returns to the panel, so the review keys keep working. It stays put if focus had already moved elsewhere. Co-Authored-By: Claude Opus 5.5 --- .../AnnotatableCodeView.browser.test.tsx | 28 +++++++++++++++- .../components/diffs/AnnotatableCodeView.tsx | 18 ++++++++++- .../PullRequestCodeTab.browser.test.tsx | 32 +++++++++++++++++++ .../pullRequest/PullRequestCodeTab.tsx | 2 +- 4 files changed, 77 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx index bb1971feeb3ad..00385422f996f 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx @@ -84,6 +84,7 @@ function SelectableCodeView(props: { files: ReturnType[]; codeViewKey: string; pendingFiles: FileDiffMetadata[]; + onDraftClose?: () => void; }) { const [selectedLines, setSelectedLines] = useState(null); return ( @@ -106,10 +107,16 @@ function view( files: ReturnType[], codeViewKey = "initial", pendingFiles: FileDiffMetadata[] = [], + onDraftClose?: () => void, ) { return ( - + ); } @@ -176,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 5ee51ac00e3c2..780ff7a9f4e8d 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -9,7 +9,14 @@ import type { } from "@pierre/diffs"; import type { CodeViewHandle } from "@pierre/diffs/react"; import type { EnvironmentId, ProjectId, ScopedThreadRef } from "@t3tools/contracts"; -import { useImperativeHandle, 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"; @@ -148,6 +155,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 ( @@ -162,6 +177,7 @@ export function AnnotatableCodeView({ ) { setDraft(null); setDraftText(""); + setDroppedDraftCount((count) => count + 1); } const items: CodeViewDiffItem[] = files.map( ({ fileDiff, filePath, fileKey, fileVersion, collapsed }) => { diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index cea33e74ffc99..9c723bbdc89da 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -6,6 +6,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"; @@ -235,3 +236,34 @@ it("never requests the diff of a selected commit the pull request no longer has" .toBeVisible(); 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!); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 63026d49429f3..bd3b7690c681b 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -847,7 +847,7 @@ function PullRequestCodeTab({ const beginComment = useStableCallback( (range: SelectedLineRange | null, context: { item: CodeViewItem }) => { - if (!range || !canSelectLines) return; + if (!range || !canSelectLines || draft !== null) return; const item = context.item; if (item.type !== "diff") return; const file = files.find((candidate) => buildFileDiffRenderKey(candidate) === item.id); From d5994e2b69318b57702eec794f65df43880a3469 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:20:38 -0700 Subject: [PATCH 21/22] fix(web): keyboard review follows the @pierre/diffs 1.7 API 1.7 rejects prepareForEditing when a refresh replaces the diff while it loads, so the line-mode warm-up now ignores that rejection; expanding loads the file itself. Tests pass parsePatchFiles its new options object. Co-Authored-By: Claude Opus 5.5 --- .../diffs/useDiffReviewKeyboard.browser.test.tsx | 2 +- apps/web/src/components/diffs/useDiffReviewKeyboard.ts | 9 +++++++-- apps/web/src/diffViewedStore.test.ts | 7 ++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 861d5a281e942..86c3b9d8b1d84 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -210,7 +210,7 @@ it("expands unchanged context after an equivalent partial diff refreshes", async id: "review", type: "diff" as const, version, - fileDiff: parsePatchFiles(patch, "refresh")[0]!.files[0]!, + fileDiff: parsePatchFiles(patch, { cacheKeyPrefix: "refresh" })[0]!.files[0]!, }, ]; await using screen = await render(); diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts index 72013d31d276c..0f3933d7cd68d 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.ts +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.ts @@ -371,8 +371,13 @@ export function useDiffReviewKeyboard(input: DiffReviewKeyboardInput 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. - if (canExpand(file)) void fileDiffInstance(file.fileKey)?.prepareForEditing(); + // 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; diff --git a/apps/web/src/diffViewedStore.test.ts b/apps/web/src/diffViewedStore.test.ts index 8f95ba70cc54f..03fafcfb7755b 100644 --- a/apps/web/src/diffViewedStore.test.ts +++ b/apps/web/src/diffViewedStore.test.ts @@ -6,7 +6,7 @@ import { diffViewedEntry, diffViewedFingerprint, useDiffViewedStore } from "./di 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}`, - "a.ts", + { cacheKeyPrefix: "a.ts" }, )[0]!.files[0]!; } @@ -31,8 +31,9 @@ describe("diffViewedFingerprint", () => { 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}`, "a.ts")[0]! - .files[0]!; + 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)), ); From 068896ec7ba5d98b0ed9e4cbc8bb7539e2116383 Mon Sep 17 00:00:00 2001 From: Christian Wooldridge <85259389+cwooldridge1@users.noreply.github.com> Date: Thu, 12 Nov 2026 10:15:16 -0700 Subject: [PATCH 22/22] test(web): drop the partial diff refresh test that main now covers main's codeViewContextExpansion.browser.test.ts (#33629) checks the same patched behavior, and this copy timed out on its first render under load. Co-Authored-By: Claude Opus 5.5 --- .../useDiffReviewKeyboard.browser.test.tsx | 33 +------------------ 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx index 86c3b9d8b1d84..76a2cb87576a6 100644 --- a/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx +++ b/apps/web/src/components/diffs/useDiffReviewKeyboard.browser.test.tsx @@ -1,11 +1,8 @@ import { parseDiffFromFile } from "@pierre/diffs"; -// oxlint-disable-next-line eslint/no-restricted-imports -- exercises hydration in the patched dependency without app theme services -import { CodeView, type CodeViewHandle } from "@pierre/diffs/react"; -import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; import { expect, it, vi } from "vite-plus/test"; import { userEvent } from "vite-plus/test/browser"; -import { act, render } from "../../../test/render"; +import { render } from "../../../test/render"; import { DiffReviewCursorContext, useDiffReviewCursorFilePath, @@ -193,31 +190,3 @@ it("leaves a file's lines past either end", async () => { await expect.element(screen.getByText("Cursor on b.ts")).toBeInTheDocument(); expect(folds).toEqual(foldsBefore); }); - -it("expands unchanged context after an equivalent partial diff refreshes", async () => { - const patch = "--- a/review.txt\n+++ b/review.txt\n@@ -48 +48 @@\n-context 48\n+changed 48\n"; - const before = Array.from({ length: 80 }, (_, index) => `context ${index + 1}\n`).join(""); - const viewer = { current: null as CodeViewHandle | null }; - const options = { - diffStyle: "unified" as const, - loadDiffFiles: async () => ({ - oldFile: { name: "review.txt", contents: before }, - newFile: { name: "review.txt", contents: before.replace("context 48", "changed 48") }, - }), - }; - const items = (version: number) => [ - { - id: "review", - type: "diff" as const, - version, - fileDiff: parsePatchFiles(patch, { cacheKeyPrefix: "refresh" })[0]!.files[0]!, - }, - ]; - await using screen = await render(); - await vi.waitFor(() => expect(viewer.current?.getInstance()?.getRenderedItems()).toHaveLength(1)); - await screen.rerender(); - const rendered = viewer.current!.getInstance()!.getRenderedItems()[0]!; - if (rendered.type !== "diff") throw new Error("Expected a diff"); - await act(() => rendered.instance.expandHunk(0, "down", 20)); - await expect.element(screen.getByText("context 47", { exact: true })).toBeVisible(); -});