Fix jumping around in pull request diff view

scm-ui/ui-components/src/index.ts

@@ -90,7 +90,7 @@ export * from "./devices";
 export { default as copyToClipboard } from "./CopyToClipboard";
 export { createA11yId } from "./createA11yId";
 export { useSecondaryNavigation } from "./useSecondaryNavigation";
-export { default as useScrollToElement } from "./useScrollToElement";
+export { default as useScrollToElement, useScrollToElementWithCallback } from "./useScrollToElement";
 export { default as DiffDropDown } from "./repos/DiffDropDown";
 
 export { default as comparators } from "./comparators";

scm-ui/ui-components/src/repos/Diff.tsx

@@ -29,7 +29,7 @@ import { getAnchorSelector, getFileNameFromHash } from "./diffs";
 import Notification from "../Notification";
 import { useTranslation } from "react-i18next";
 import { useLocation } from "react-router-dom";
-import useScrollToElement from "../useScrollToElement";
+import { useScrollToElementWithCallback } from "../useScrollToElement";
 import { getAnchorId } from "./diff/helpers";
 
 type Props = DiffObjectProps & {
@@ -39,6 +39,8 @@ type Props = DiffObjectProps & {
   fetchNextPage?: () => void;
   isFetchingNextPage?: boolean;
   isDataPartial?: boolean;
+  prevHash?: string;
+  setPrevHash?: (newHash: string) => void;
 };
 
 const createKey = (file: FileDiff, ignoreWhitespace?: string) => {
@@ -66,6 +68,8 @@ const Diff: FC<Props> = ({
   fetchNextPage,
   isFetchingNextPage,
   isDataPartial,
+  prevHash,
+  setPrevHash,
   ...fileProps
 }) => {
   const [t] = useTranslation("repos");
@@ -78,13 +82,17 @@ const Diff: FC<Props> = ({
     }
   }, [isFetchingNextPage]);
 
-  useScrollToElement(
+  useScrollToElementWithCallback(
     contentRef,
     () => {
       if (isFetchingNextPage === undefined || isDataPartial === undefined || fetchNextPage === undefined) {
         return selectFromHash(hash);
       }
 
+      if (prevHash === hash) {
+        return;
+      }
+
       const encodedFileName = getFileNameFromHash(hash);
       if (!encodedFileName) {
         return;
@@ -99,7 +107,12 @@ const Diff: FC<Props> = ({
         fetchNextPage();
       }
     },
-    [hash]
+    [hash, isDataPartial, isFetchingNextPage, fetchNextPage, diff, prevHash, setPrevHash],
+    () => {
+      if (setPrevHash) {
+        setPrevHash(hash);
+      }
+    }
   );
 
   return (

scm-ui/ui-components/src/repos/LoadingDiff.tsx

@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-import React, { FC, useCallback, useState } from "react";
+import React, { FC, useState } from "react";
 import { useTranslation } from "react-i18next";
 import { NotFoundError, useDiff } from "@scm-manager/ui-api";
 import ErrorNotification from "../ErrorNotification";
@@ -33,7 +33,6 @@ import { DiffObjectProps } from "./DiffTypes";
 import DiffStatistics from "./DiffStatistics";
 import { DiffDropDown } from "../index";
 import DiffFileTree from "./diff/DiffFileTree";
-import { FileTree } from "@scm-manager/ui-types";
 import { DiffContent, FileTreeContent } from "./diff/styledElements";
 import { useHistory, useLocation } from "react-router-dom";
 import { getFileNameFromHash } from "./diffs";
@@ -65,14 +64,10 @@ const PartialNotification: FC<NotificationProps> = ({ fetchNextPage, isFetchingN
 const LoadingDiff: FC<Props> = ({ url, limit, refetchOnWindowFocus, ...props }) => {
   const [ignoreWhitespace, setIgnoreWhitespace] = useState(false);
   const [collapsed, setCollapsed] = useState(false);
+  const [prevHash, setPrevHash] = useState("");
   const location = useLocation();
   const history = useHistory();
 
-  const fetchNextPageAndResetAnchor = () => {
-    history.push("#");
-    fetchNextPage();
-  };
-
   const evaluateWhiteSpace = () => {
     return ignoreWhitespace ? "ALL" : "NONE";
   };
@@ -83,26 +78,6 @@ const LoadingDiff: FC<Props> = ({ url, limit, refetchOnWindowFocus, ...props })
   });
   const [t] = useTranslation("repos");
 
-  const getFirstFile = useCallback((tree: FileTree): string => {
-    if (Object.keys(tree.children).length === 0) {
-      return tree.nodeName;
-    }
-
-    for (const key in tree.children) {
-      let path;
-      if (tree.nodeName !== "") {
-        path = tree.nodeName + "/";
-      } else {
-        path = tree.nodeName;
-      }
-      const result = path + getFirstFile(tree.children[key]);
-      if (result) {
-        return result;
-      }
-    }
-    return "";
-  }, []);
-
   const ignoreWhitespaces = () => {
     setIgnoreWhitespace(!ignoreWhitespace);
   };
@@ -112,6 +87,7 @@ const LoadingDiff: FC<Props> = ({ url, limit, refetchOnWindowFocus, ...props })
   };
 
   const setFilePath = (path: string) => {
+    setPrevHash("");
     history.push(`#diff-${encodeURIComponent(path)}`);
   };
 
@@ -148,10 +124,12 @@ const LoadingDiff: FC<Props> = ({ url, limit, refetchOnWindowFocus, ...props })
             fetchNextPage={fetchNextPage}
             isFetchingNextPage={isFetchingNextPage}
             isDataPartial={data.partial}
+            prevHash={prevHash}
+            setPrevHash={setPrevHash}
             {...props}
           />
           {data.partial ? (
-            <PartialNotification fetchNextPage={fetchNextPageAndResetAnchor} isFetchingNextPage={isFetchingNextPage} />
+            <PartialNotification fetchNextPage={fetchNextPage} isFetchingNextPage={isFetchingNextPage} />
           ) : null}
         </DiffContent>
       </div>

scm-ui/ui-components/src/useScrollToElement.ts

@@ -24,10 +24,12 @@
 
 import { useEffect } from "react";
 
-const useScrollToElement = (
+// In some cases it is necessary to execute some logic, after successfully jumping to an element
+export const useScrollToElementWithCallback = (
   contentRef: HTMLElement | null | undefined,
   elementSelectorResolver: () => string | undefined,
-  ...dependencies: any
+  dependencies: unknown[],
+  callback?: () => void
 ) => {
   useEffect(() => {
     const elementSelector = elementSelectorResolver();
@@ -46,6 +48,10 @@ const useScrollToElement = (
             clearInterval(intervalId);
             const y = element.getBoundingClientRect().top + window.pageYOffset - margin;
             window.scrollTo({ top: y });
+
+            if (callback) {
+              callback();
+            }
           }
         }
       }, 200);
@@ -57,6 +63,14 @@ const useScrollToElement = (
   }, [contentRef, ...dependencies]);
 };
 
+const useScrollToElement = (
+  contentRef: HTMLElement | null | undefined,
+  elementSelectorResolver: () => string | undefined,
+  ...dependencies: any
+) => {
+  useScrollToElementWithCallback(contentRef, elementSelectorResolver, [...dependencies]);
+};
+
 function escapeIdStartingWithNumber(selector: string) {
   if (selector.startsWith("#")) {
     return `#${CSS.escape(selector.substring(1))}`;