From ac80dcf26457a93b4b40c12736b8c137f080e0e7 Mon Sep 17 00:00:00 2001 From: Dinesh Pandiyan Date: Tue, 27 Oct 2026 15:29:54 +1100 Subject: [PATCH] fix: keep reviewed file header in place so next diff starts at top --- DESIGN.md | 2 ++ apps/web/src/DiffWorkspace.tsx | 25 ++++++++++++++- apps/web/src/use-reviewed-files.ts | 9 ++++-- apps/web/test/browser/app.spec.ts | 50 ++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 4 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 60bd789f..7f2bc38c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -539,6 +539,8 @@ at `/design.html` (see `docs/development.md`). and Alt+J/K. Marking the file in view hides it at once and moves selection to its next visible neighbor. Empty results say “All files reviewed” with a “Show reviewed files” action; comment navigation to a hidden file shows them. + When shown, marking a file scrolled past its top keeps its collapsed header + at the viewport top, so the next file starts from its first line. - Navigating from the file list briefly pulses the destination file header with the accent for about one second. Reduced-motion mode uses static feedback that remains visible long enough to identify the destination without animation. diff --git a/apps/web/src/DiffWorkspace.tsx b/apps/web/src/DiffWorkspace.tsx index 5b0ba868..db9cc671 100644 --- a/apps/web/src/DiffWorkspace.tsx +++ b/apps/web/src/DiffWorkspace.tsx @@ -189,6 +189,7 @@ export function DiffWorkspace() { navigationTarget, commentNavigationTarget, navigateComment, + hideReviewed, viewer, } = useNavigation(); const { isReviewed, toggleReviewed, pending } = useReviewedFilesState(); @@ -783,7 +784,29 @@ export function DiffWorkspace() { aria-pressed={isReviewed(file)} disabled={pending.has(file.id) || pending.has("*")} aria-busy={pending.has(file.id)} - onClick={() => toggleReviewed(file)} + onClick={async () => { + // Marking a file scrolled past its top collapses it above + // the viewport; restore its header there so the next file + // shows from its top. Hidden files hand off in navigation. + const instance = viewer.current?.getInstance(); + const top = instance?.getTopForItem(file.path); + const scrolledInto = + !hideReviewed && + !isReviewed(file) && + top !== undefined && + instance !== undefined && + instance.getScrollTop() > top; + if (!(await toggleReviewed(file)) || !scrolledInto) + return; + requestAnimationFrame(() => { + viewer.current?.scrollTo({ + type: "item", + id: file.path, + align: "start", + behavior: "instant", + }); + }); + }} > { const owner = requests.current; const currentRepository = latest.current.repository; - if (owner?.key !== key || currentRepository?.id !== diffId) return; + if (owner?.key !== key || currentRepository?.id !== diffId) return false; const request = owner.beginWrite(file.id); - if (!request) return; + if (!request) return false; const marked = latest.current.entries.get(file.id) === file.fingerprint; setPending((current) => new Set(current).add(file.id)); setFailure(null); @@ -130,7 +131,7 @@ export function useReviewedFiles( signal: request.signal, }, ); - if (!request.isCurrent()) return; + if (!request.isCurrent()) return false; if (!result.response.ok) throw new Error( errorDetail(result.error, "Unable to update reviewed file"), @@ -149,12 +150,14 @@ export function useReviewedFiles( else next.add(file.path); return next; }); + return true; } catch (cause) { if (request.isCurrent()) setFailure({ kind: "write", message: errorDetail(cause, "Unable to update reviewed file"), }); + return false; } finally { const current = request.isCurrent(); request.finish(); diff --git a/apps/web/test/browser/app.spec.ts b/apps/web/test/browser/app.spec.ts index 1611d45d..15656796 100644 --- a/apps/web/test/browser/app.spec.ts +++ b/apps/web/test/browser/app.spec.ts @@ -951,6 +951,56 @@ test("reset reviewed clears file review marks and disables when empty", async ({ await expect(reset).toBeDisabled(); }); +test("marking a file reviewed mid-scroll shows the next file from its top", async ({ + page, +}) => { + // The file is taller than the viewer, so its next file starts out of view. + await page.setViewportSize({ width: 1280, height: 260 }); + await page.goto("/"); + await showDiff(page); + const path = "src/hooks/useClipboard.ts"; + const next = "src/styles/theme.css"; + const viewer = page.locator("#viewer"); + const file = (name: string) => + viewer.locator("diffs-container").filter({ hasText: name }); + // Sticky headers always sit at the top, so measure whole files instead. + const top = async (name: string) => { + const [viewerBox, fileBox] = await Promise.all([ + viewer.boundingBox(), + file(name).boundingBox(), + ]); + return viewerBox && fileBox + ? fileBox.y - viewerBox.y + : Number.POSITIVE_INFINITY; + }; + + await page.locator(`#file-tree [data-path="${path}"]`).click(); + await expect + .poll(async () => Math.abs(await top(path))) + .toBeLessThanOrEqual(1); + await page.locator(".diff-code-view").evaluate((element) => { + element.scrollTop += 60; + }); + expect(await top(path)).toBeLessThan(-50); + // A pointer click would first scroll the sticky toggle to its natural place. + await page + .getByRole("button", { name: `Mark ${path} reviewed` }) + .evaluate((element) => { + if (element instanceof HTMLElement) element.click(); + }); + await expect( + page.getByRole("button", { name: `Mark ${path} unreviewed` }), + ).toBeVisible(); + // The collapsed file keeps its header in place and the next file follows it. + await expect + .poll(async () => Math.abs(await top(path))) + .toBeLessThanOrEqual(1); + const collapsed = await file(path).boundingBox(); + if (!collapsed) throw new Error("Missing collapsed file"); + expect(await top(next)).toBeGreaterThanOrEqual(collapsed.height); + expect(await top(next)).toBeLessThan(collapsed.height + 20); +}); + test("hide reviewed files filters the tree and diff together", async ({ page, }) => {