From c863871c70617c397223cc0fea6c7f0431e51ede Mon Sep 17 00:00:00 2001 From: Xin Date: Sun, 18 Oct 2026 21:34:33 +0100 Subject: [PATCH 1/6] fix(web): advance to next file after marking reviewed with m Marking a file reviewed via the keyboard now scrolls to the next file instead of restoring a stale scroll offset (which could land at the top or on an unrelated file when hide-reviewed remounts the view). Also widen the current-file header tolerance so a pinned header isn't mistaken for the previous file, which made m mark the wrong file. --- web/src/components/DiffView.tsx | 90 ++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 36 deletions(-) diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index e697268..13aee65 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -105,6 +105,9 @@ const codeViewStyle = { // Window after a programmatic scroll during which the cursor isn't re-synced. const PROGRAMMATIC_SCROLL_SETTLE_MS = 250; +// A pinned file header sits a few px below the scroll area's top; the next file's +// header is at least a header height lower until it's scrolled to. +const CURRENT_FILE_HEADER_SLOP_PX = 24; const EDITABLE_TAGS = /^(INPUT|TEXTAREA|SELECT)$/; type Viewer = CodeViewHandle; @@ -352,6 +355,8 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" const codeViewAreaRef = useRef(null); const currentFileRef = useRef(null); const programmaticScrollAtRef = useRef(0); + // File to scroll to once CodeView remounts, in place of the saved scroll offset. + const pendingScrollFileRef = useRef(null); const [commentThreads, setCommentThreads] = useState([]); const [commentTarget, setCommentTarget] = useState(null); const [selectedLines, setSelectedLines] = useState(null); @@ -679,11 +684,27 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" const areaTop = area.getBoundingClientRect().top; let current = headers[0].getAttribute("data-diff-file"); for (let i = 1; i < headers.length; i++) { - if (headers[i].getBoundingClientRect().top - areaTop > 8) break; + if (headers[i].getBoundingClientRect().top - areaTop > CURRENT_FILE_HEADER_SLOP_PX) break; current = headers[i].getAttribute("data-diff-file"); } return current; }, []); + const scrollToFile = useCallback( + (path: string, behavior: CodeViewScrollBehavior = "smooth-auto") => { + const itemId = filePathToItemId.get(path); + if (itemId == null) return; + currentFileRef.current = path; + programmaticScrollAtRef.current = Date.now(); + viewerRef.current?.scrollTo({ + type: "item", + id: itemId, + align: "start", + behavior, + }); + setMobileSidebarOpen(false); + }, + [filePathToItemId], + ); useEffect(() => { const area = codeViewAreaRef.current; if (!area) return; @@ -708,17 +729,21 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" }; area.addEventListener("scroll", handleScroll, { capture: true, passive: true }); - const saved = sessionStorage.getItem(scrollStorageKey); - if (saved) { - const target = parseInt(saved, 10); - if (target > 0) { + // A pending file wins: after hiding a reviewed file the saved offset points elsewhere. + const pendingFile = pendingScrollFileRef.current; + pendingScrollFileRef.current = null; + const saved = parseInt(sessionStorage.getItem(scrollStorageKey) ?? "", 10); + if (pendingFile != null || saved > 0) { + requestAnimationFrame(() => { requestAnimationFrame(() => { - requestAnimationFrame(() => { - const scrollEl = area.firstElementChild as HTMLElement; - if (scrollEl) scrollEl.scrollTop = target; - }); + if (pendingFile != null) { + scrollToFile(pendingFile, "instant"); + return; + } + const scrollEl = area.firstElementChild as HTMLElement; + if (scrollEl) scrollEl.scrollTop = saved; }); - } + }); } return () => { @@ -726,23 +751,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" if (timer) clearTimeout(timer); if (rafId) cancelAnimationFrame(rafId); }; - }, [scrollStorageKey, codeViewKey, getCurrentFile]); - const scrollToFile = useCallback( - (path: string, behavior: CodeViewScrollBehavior = "smooth-auto") => { - const itemId = filePathToItemId.get(path); - if (itemId == null) return; - currentFileRef.current = path; - programmaticScrollAtRef.current = Date.now(); - viewerRef.current?.scrollTo({ - type: "item", - id: itemId, - align: "start", - behavior, - }); - setMobileSidebarOpen(false); - }, - [filePathToItemId], - ); + }, [scrollStorageKey, codeViewKey, getCurrentFile, scrollToFile]); const scrollToThread = useCallback( (thread: ReviewThread) => { const itemId = filePathToItemId.get(thread.path); @@ -847,17 +856,25 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" if (current == null) return; const itemId = filePathToItemId.get(current); if (itemId == null) return; - // Marking hides the file when "hide reviewed" is on; pre-advance the cursor. + const sig = fileSignatures.get(current); + const marking = sig != null && reviewed.map.get(current) !== sig; + toggleReviewed(itemId); + if (!marking) return; + // Advance to the next file; on the last file, stay on it (or step back if it gets hidden). + const idx = visibleFiles.findIndex((f) => f.name === current); + const next = + visibleFiles[idx + 1] ?? (hideReviewed ? visibleFiles[idx - 1] : visibleFiles[idx]); + if (next == null) return; + currentFileRef.current = next.name; if (hideReviewed) { - const sig = fileSignatures.get(current); - const willHide = sig != null && reviewed.map.get(current) !== sig; - if (willHide) { - const idx = visibleFiles.findIndex((f) => f.name === current); - const next = visibleFiles[idx + 1] ?? visibleFiles[idx - 1]; - currentFileRef.current = next?.name ?? null; - } + // Hiding remounts CodeView; the scroll-restore effect scrolls once it's mounted. + pendingScrollFileRef.current = next.name; + } else { + // Wait for the collapse to re-measure, or the target offset is stale. + requestAnimationFrame(() => { + requestAnimationFrame(() => scrollToFile(next.name, "instant")); + }); } - toggleReviewed(itemId); }, [ getCurrentFile, filePathToItemId, @@ -866,6 +883,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" fileSignatures, reviewed, visibleFiles, + scrollToFile, ]); const onKeyDown = useEffectEvent((e: KeyboardEvent) => { if (e.metaKey || e.ctrlKey || e.altKey) return; From 196c0a6c17c19eaae3d911a293d46f378211e03c Mon Sep 17 00:00:00 2001 From: Xin Date: Sun, 18 Oct 2026 22:31:59 +0100 Subject: [PATCH 2/6] feat(web): use r to mark a file reviewed, keep m as a silent alias r matches the Reviewed button label; m still works but is no longer listed. --- web/src/components/DiffView.tsx | 5 +++-- web/src/components/diff-view/ShortcutsDialog.tsx | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index 13aee65..4a229bc 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -908,7 +908,8 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" e.preventDefault(); navigateFile(-1); break; - case "m": + case "r": + case "m": // Undocumented alias, kept for existing muscle memory. e.preventDefault(); reviewCurrentFile(); break; @@ -1348,7 +1349,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" return (