Fixed missing update if content of diff changes (#1714)

* Fixed missing update if content of diff changes

* Add property to disable automatic refetch in diffs

Co-authored-by: René Pfeuffer <rene.pfeuffer@cloudogu.com>

gradle/changelog/diff_disable_refetch_on_focus.yaml

@@ -0,0 +1,2 @@
+- type: added
+  description: Option to diable automatic refresh for diff view on window focus change ([#1714](https://github.com/scm-manager/scm-manager/pull/1714))

gradle/changelog/diff_update_content.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Missing update if content of diff changes ([#1714](https://github.com/scm-manager/scm-manager/pull/1714))

scm-ui/ui-api/src/diff.ts

@@ -30,9 +30,14 @@ import { Diff, Link } from "@scm-manager/ui-types";
 
 type UseDiffOptions = {
   limit?: number;
+  refetchOnWindowFocus?: boolean;
 };
 
-export const useDiff = (link: string, options: UseDiffOptions = {}) => {
+const defaultOptions: UseDiffOptions = {
+  refetchOnWindowFocus: true,
+};
+
+export const useDiff = (link: string, options: UseDiffOptions = defaultOptions) => {
   let initialLink = link;
   if (options.limit) {
     const separator = initialLink.includes("?") ? "&" : "?";
@@ -41,7 +46,7 @@ export const useDiff = (link: string, options: UseDiffOptions = {}) => {
   const { isLoading, error, data, isFetchingNextPage, fetchNextPage } = useInfiniteQuery<Diff, Error, Diff>(
     ["link", link],
     ({ pageParam }) => {
-      return apiClient.get(pageParam || initialLink).then(response => {
+      return apiClient.get(pageParam || initialLink).then((response) => {
         const contentType = response.headers.get("Content-Type");
         if (contentType && contentType.toLowerCase() === "application/vnd.scmm-diffparsed+json;v=2") {
           return response.json();
@@ -49,18 +54,19 @@ export const useDiff = (link: string, options: UseDiffOptions = {}) => {
           return response
             .text()
             .then(parser.parse)
-            .then(parsedGit => {
+            .then((parsedGit) => {
               return {
                 files: parsedGit,
                 partial: false,
-                _links: {}
+                _links: {},
               };
             });
         }
       });
     },
     {
-      getNextPageParam: lastPage => (lastPage._links.next as Link)?.href
+      getNextPageParam: (lastPage) => (lastPage._links.next as Link)?.href,
+      refetchOnWindowFocus: options.refetchOnWindowFocus,
     }
   );
 
@@ -71,7 +77,7 @@ export const useDiff = (link: string, options: UseDiffOptions = {}) => {
     fetchNextPage: () => {
       fetchNextPage();
     },
-    data: merge(data?.pages)
+    data: merge(data?.pages),
   };
 };
 
@@ -79,9 +85,9 @@ const merge = (diffs?: Diff[]): Diff | undefined => {
   if (!diffs || diffs.length === 0) {
     return;
   }
-  const joinedFiles = diffs.flatMap(diff => diff.files);
+  const joinedFiles = diffs.flatMap((diff) => diff.files);
   return {
     ...diffs[diffs.length - 1],
-    files: joinedFiles
+    files: joinedFiles,
   };
 };

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

@@ -36,9 +36,10 @@ import { getPath } from "./diffs";
 import DiffButton from "./DiffButton";
 import styled from "styled-components";
 import { MemoryRouter } from "react-router-dom";
-import { one, two } from "../__resources__/changesets";
+import { two } from "../__resources__/changesets";
 import { Changeset, FileDiff } from "@scm-manager/ui-types";
 import JumpToFileButton from "./JumpToFileButton";
+import Button from "../buttons/Button";
 
 const diffFiles = parser.parse(simpleDiff);
 
@@ -48,15 +49,15 @@ const Container = styled.div`
 
 const RoutingDecorator = (story: () => ReactNode) => <MemoryRouter initialEntries={["/"]}>{story()}</MemoryRouter>;
 
-const fileControlFactory: (changeset: Changeset) => FileControlFactory = changeset => file => {
+const fileControlFactory: (changeset: Changeset) => FileControlFactory = (changeset) => (file) => {
   const baseUrl = "/repo/hitchhiker/heartOfGold/code/changeset";
   const sourceLink = {
     url: `${baseUrl}/${changeset.id}/${file.newPath}/`,
-    label: "Jump to source"
+    label: "Jump to source",
   };
   const targetLink = changeset._embedded?.parents?.length === 1 && {
     url: `${baseUrl}/${changeset._embedded.parents[0].id}/${file.oldPath}`,
-    label: "Jump to target"
+    label: "Jump to target",
   };
 
   const links = [];
@@ -82,7 +83,7 @@ const fileControlFactory: (changeset: Changeset) => FileControlFactory = changes
 
 storiesOf("Diff", module)
   .addDecorator(RoutingDecorator)
-  .addDecorator(storyFn => <Container>{storyFn()}</Container>)
+  .addDecorator((storyFn) => <Container>{storyFn()}</Container>)
   .add("Default", () => <Diff diff={diffFiles} />)
   .add("Side-By-Side", () => <Diff diff={diffFiles} sideBySide={true} />)
   .add("Collapsed", () => <Diff diff={diffFiles} defaultCollapse={true} fileControlFactory={fileControlFactory(two)} />)
@@ -101,15 +102,15 @@ storiesOf("Diff", module)
   .add("File Annotation", () => (
     <Diff
       diff={diffFiles}
-      fileAnnotationFactory={file => [<p key={file.newPath}>Custom File annotation for {file.newPath}</p>]}
+      fileAnnotationFactory={(file) => [<p key={file.newPath}>Custom File annotation for {file.newPath}</p>]}
     />
   ))
   .add("Line Annotation", () => (
     <Diff
       diff={diffFiles}
-      annotationFactory={ctx => {
+      annotationFactory={(ctx) => {
         return {
-          N2: <p key="N2">Line Annotation</p>
+          N2: <p key="N2">Line Annotation</p>,
         };
       }}
     />
@@ -166,4 +167,20 @@ storiesOf("Diff", module)
     });
     return <Diff diff={filesWithLanguage} />;
   })
-  .add("WithLinkToFile", () => <Diff diff={diffFiles} />);
+  .add("WithLinkToFile", () => <Diff diff={diffFiles} />)
+  .add("Changing Content", () => {
+    const ChangingContentDiff = () => {
+      const [markdown, setMarkdown] = useState(false);
+      return (
+        <div>
+          <Button className="mb-5" action={() => setMarkdown((m) => !m)}>
+            Change content
+          </Button>
+          {/* @ts-ignore */}
+          <Diff diff={markdown ? markdownDiff.files : diffFiles} />
+        </div>
+      );
+    };
+
+    return <ChangingContentDiff />;
+  });

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

@@ -21,78 +21,57 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-import React from "react";
+import React, { FC, useState } from "react";
 import DiffFile from "./DiffFile";
 import { DiffObjectProps, FileControlFactory } from "./DiffTypes";
 import { FileDiff } from "@scm-manager/ui-types";
 import { escapeWhitespace } from "./diffs";
 import Notification from "../Notification";
-import { WithTranslation, withTranslation } from "react-i18next";
-import { RouteComponentProps, withRouter } from "react-router-dom";
+import { useTranslation } from "react-i18next";
+import { useLocation } from "react-router-dom";
+import useScrollToElement from "../useScrollToElement";
 
-type Props = RouteComponentProps &
-  WithTranslation &
-  DiffObjectProps & {
+type Props = DiffObjectProps & {
   diff: FileDiff[];
   fileControlFactory?: FileControlFactory;
-  };
-
-type State = {
-  contentRef?: HTMLElement | null;
 };
 
-function getAnchorSelector(uriHashContent: string) {
+const createKey = (file: FileDiff) => {
+  return `${file.oldPath}@${file.oldRevision}/${file.newPath}@${file.newRevision}`;
+};
+
+const getAnchorSelector = (uriHashContent: string) => {
   return "#" + escapeWhitespace(decodeURIComponent(uriHashContent));
-}
+};
 
-class Diff extends React.Component<Props, State> {
-  static defaultProps: Partial<Props> = {
-    sideBySide: false
-  };
-
-  constructor(props: Readonly<Props>) {
-    super(props);
-    this.state = {
-      contentRef: undefined
-    };
+const Diff: FC<Props> = ({ diff, ...fileProps }) => {
+  const [t] = useTranslation("repos");
+  const [contentRef, setContentRef] = useState<HTMLElement | null>();
+  const { hash } = useLocation();
+  useScrollToElement(
+    contentRef,
+    () => {
+      const match = hash.match(/^#diff-(.*)$/);
+      if (match) {
+        return getAnchorSelector(match[1]);
       }
-
-  componentDidUpdate() {
-    const { contentRef } = this.state;
-
-    // we have to use componentDidUpdate, because we have to wait until all
-    // children are rendered and componentDidMount is called before the
-    // changeset content was rendered.
-    const hash = this.props.location.hash;
-    const match = hash && hash.match(/^#diff-(.*)$/);
-    if (contentRef && match) {
-      const selector = getAnchorSelector(match[1]);
-      const element = contentRef.querySelector(selector);
-      if (element && element.scrollIntoView) {
-        element.scrollIntoView();
-      }
-    }
-  }
-
-  shouldComponentUpdate(nextProps: Readonly<Props>, nextState: Readonly<State>): boolean {
-    // We have check if the contentRef changed and update afterwards so the page can scroll to the anchor links.
-    // Otherwise it can happen that componentDidUpdate is never executed depending on how fast the markdown got rendered
-    return this.state.contentRef !== nextState.contentRef || this.props !== nextProps;
-  }
-
-  render() {
-    const { diff, t, ...fileProps } = this.props;
+    },
+    hash
+  );
 
   return (
-      <div ref={el => this.setState({ contentRef: el })}>
+    <div ref={setContentRef}>
       {diff.length === 0 ? (
         <Notification type="info">{t("diff.noDiffFound")}</Notification>
       ) : (
-          diff.map((file, index) => <DiffFile key={index} file={file} {...fileProps} {...this.props} />)
+        diff.map((file) => <DiffFile key={createKey(file)} file={file} {...fileProps} />)
       )}
     </div>
   );
-  }
-}
+};
 
-export default withRouter(withTranslation("repos")(Diff));
+Diff.defaultProps = {
+  sideBySide: false,
+};
+
+export default Diff;

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

@@ -36,6 +36,7 @@ import styled from "styled-components";
 type Props = DiffObjectProps & {
   url: string;
   limit?: number;
+  refetchOnWindowFocus?: boolean;
 };
 
 type NotificationProps = {
@@ -59,8 +60,8 @@ const PartialNotification: FC<NotificationProps> = ({ fetchNextPage, isFetchingN
   );
 };
 
-const LoadingDiff: FC<Props> = ({ url, limit, ...props }) => {
-  const { error, isLoading, data, fetchNextPage, isFetchingNextPage } = useDiff(url, { limit });
+const LoadingDiff: FC<Props> = ({ url, limit, refetchOnWindowFocus, ...props }) => {
+  const { error, isLoading, data, fetchNextPage, isFetchingNextPage } = useDiff(url, { limit, refetchOnWindowFocus });
   const [t] = useTranslation("repos");
 
   if (error) {
@@ -86,7 +87,8 @@ const LoadingDiff: FC<Props> = ({ url, limit, ...props }) => {
 
 LoadingDiff.defaultProps = {
   limit: 25,
-  sideBySide: false
+  sideBySide: false,
+  refetchOnWindowFocus: true,
 };
 
 export default LoadingDiff;