From 8b3aadbd480f05d32b7f874b33119ac9dacdb354 Mon Sep 17 00:00:00 2001
From: Greyson Parrelli <greyson@signal.org>
Date: Fri, 4 Sep 2026 14:27:24 -0400
Subject: [PATCH] Fix scroll position after folding in review tool.

---
 web/src/components/DiffView.tsx | 65 ++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx
index d357730..da42582 100644
--- a/web/src/components/DiffView.tsx
+++ b/web/src/components/DiffView.tsx
@@ -2,6 +2,7 @@ import {
   memo,
   useCallback,
   useEffect,
+  useLayoutEffect,
   useMemo,
   useRef,
   useState,
@@ -139,6 +140,27 @@ function useCommentsByFile(
   }, [comments, outdated]);
 }
 
+// The scrolling ancestor an element sits in. Found by asking the layout rather
+// than by naming `.main`, so moving the scroll port can't quietly turn the fold
+// correction below into a no-op.
+function scrollPortOf(el: Element): Element {
+  for (let p = el.parentElement; p; p = p.parentElement) {
+    const overflow = getComputedStyle(p).overflowY;
+    if (overflow === 'auto' || overflow === 'scroll') return p;
+  }
+  return document.scrollingElement ?? document.documentElement;
+}
+
+// Where a scroll port's top edge is in viewport coordinates. The document's own
+// scrolling element is the exception: its box moves with the scroll, so its rect
+// is not where the reader's view of it starts.
+function portTopOf(port: Element): number {
+  if (port === document.scrollingElement || port === document.documentElement) {
+    return 0;
+  }
+  return port.getBoundingClientRect().top;
+}
+
 export function DiffView({
   files,
   comments,
@@ -286,6 +308,45 @@ const FileView = memo(function FileView({
     setScope(hasPrevious ? 'since' : 'full');
   }, [hasPrevious]);
 
+  // Set when a fold needs the scroll position corrected once it has landed, and
+  // cleared by the layout effect that does it. A ref because it is a note to the
+  // next render rather than anything worth re-rendering over.
+  const refoldRef = useRef(false);
+
+  // Folding a file you are part-way through takes its height out from *above*
+  // the viewport, and the browser leaves scrollTop where it was — so the diff
+  // slides forward under you and the header you were just reading ends up back
+  // up the page. Put the card's top where its sticky header already was: the top
+  // of the scroll port.
+  //
+  // In a layout effect rather than in the click handler because the height has
+  // to be gone before the correction can be measured, and gone before the
+  // browser paints so the jump is never seen.
+  useLayoutEffect(() => {
+    if (!refoldRef.current) return;
+    refoldRef.current = false;
+    document.getElementById(`file-${path}`)?.scrollIntoView({ block: 'start' });
+  }, [collapsed, path]);
+
+  // The one way in for both the chevron and the viewed checkbox: either can be
+  // clicked from a header stuck to the top of the port with the file's own top
+  // off the screen, and the fold moves the page the same way whichever did it.
+  const fold = (next: boolean) => {
+    // Already folded the way it is asked to be — nothing moves, and arming the
+    // correction below would leave it armed for a later fold to fire on, since
+    // the layout effect only runs when `collapsed` actually changes.
+    if (next === collapsed) return;
+
+    const card = next ? document.getElementById(`file-${path}`) : null;
+    // Only when the card's top has scrolled off. Folding one you can see whole
+    // moves nothing above the viewport, so there is nothing to correct — and
+    // scrolling anyway would drag the page for no reason.
+    refoldRef.current =
+      card != null &&
+      card.getBoundingClientRect().top < portTopOf(scrollPortOf(card));
+    setCollapsed(next);
+  };
+
   // A jump from the comments rail can target a thread inside a collapsed file;
   // opening the file here is what puts that thread in the DOM for the scroll to
   // find. Keyed on the request's sequence number, so clicking the same comment
@@ -565,7 +626,7 @@ const FileView = memo(function FileView({
   // just won't be able to show you what moved.
   const toggleViewed = () => {
     onSetViewed(path, !viewed, newSource ?? undefined);
-    setCollapsed(!viewed);
+    fold(!viewed);
   };
 
   return (
@@ -577,7 +638,7 @@ const FileView = memo(function FileView({
       >
         <button
           className="file-collapse"
-          onClick={() => setCollapsed((v) => !v)}
+          onClick={() => fold(!collapsed)}
           aria-label={collapsed ? 'Expand' : 'Collapse'}
         >
           <Icon name={collapsed ? 'chevron-right' : 'chevron-down'} />