fix endless loop, by using the same reference for the branch array

scm-ui/src/repos/containers/BranchRoot.js

@@ -18,6 +18,7 @@ import { compose } from "redux";
 type Props = {
   repository: Repository,
   baseUrl: string,
+  selected: string,
 
   // State props
   branches: Branch[],
@@ -31,37 +32,16 @@ type Props = {
   match: any
 };
 
-type State = {
-  selectedBranch?: Branch
-};
-
-class BranchRoot extends React.Component<Props, State> {
+class BranchRoot extends React.Component<Props> {
   constructor(props: Props) {
     super(props);
     this.state = {};
   }
 
   componentDidMount() {
-    console.log("BR did mount");
     this.props.fetchBranches(this.props.repository);
   }
 
-  componentDidUpdate(prevProps: Props) {
-    const { branches, match, loading } = this.props;
-    console.log("BR did update");
-    const branchName = decodeURIComponent(match.params.branch);
-    if (branches) {
-      if (
-        (!loading && prevProps.loading) ||
-        match.url !== prevProps.match.url
-      ) {
-        this.setState({
-          selectedBranch: branches.find(b => b.name === branchName)
-        });
-      }
-    }
-  }
-
   stripEndingSlash = (url: string) => {
     if (url.endsWith("/")) {
       return url.substring(0, url.length - 1);
@@ -80,9 +60,14 @@ class BranchRoot extends React.Component<Props, State> {
     );
   };
 
+  findSelectedBranch = () => {
+    const { selected, branches } = this.props;
+    return branches.find((branch: Branch) => branch.name === selected);
+  };
+
   render() {
-    console.log("BR render");
-    const { repository, match, branches, loading } = this.props;
+    // TODO error???
+    const { repository, loading, match, branches } = this.props;
     const url = this.stripEndingSlash(match.url);
 
     if (loading) {
@@ -92,6 +77,9 @@ class BranchRoot extends React.Component<Props, State> {
       return null;
     }
 
+    const branch = this.findSelectedBranch();
+    const changesets = <Changesets repository={repository} branch={branch} />;
+
     return (
       <>
         <BranchSelector
@@ -100,15 +88,7 @@ class BranchRoot extends React.Component<Props, State> {
             this.branchSelected(b);
           }}
         />
-        <Route
-          path={`${url}/changesets/:page?`}
-          component={() => (
-            <Changesets
-              repository={repository}
-              branch={this.state.selectedBranch}
-            />
-          )}
-        />
+        <Route path={`${url}/:page?`} component={() => changesets} />
       </>
     );
   }
@@ -123,15 +103,17 @@ const mapDispatchToProps = dispatch => {
 };
 
 const mapStateToProps = (state: any, ownProps: Props) => {
-  const { repository } = ownProps;
+  const { repository, match } = ownProps;
   const loading = isFetchBranchesPending(state, repository);
   const error = getFetchBranchesFailure(state, repository);
-
   const branches = getBranches(state, repository);
+  const selected = decodeURIComponent(match.params.branch);
+
   return {
     loading,
     error,
-    branches
+    branches,
+    selected
   };
 };
 

scm-ui/src/repos/containers/BranchSelector.js

@@ -10,13 +10,16 @@ type Props = {
 };
 
 type State = { selectedBranch?: Branch };
+
 class BranchSelector extends React.Component<Props, State> {
   constructor(props: Props) {
     super(props);
     this.state = {};
   }
+
   render() {
     const { branches } = this.props;
+
     if (branches) {
       return (
         <>

scm-ui/src/repos/containers/Changesets.js

@@ -1,8 +1,13 @@
 // @flow
 
 import React from "react";
-import {withRouter} from "react-router-dom";
-import type {Branch, Changeset, PagedCollection, Repository} from "@scm-manager/ui-types";
+import { withRouter } from "react-router-dom";
+import type {
+  Branch,
+  Changeset,
+  PagedCollection,
+  Repository
+} from "@scm-manager/ui-types";
 import {
   fetchChangesetsByBranch,
   fetchChangesetsByBranchAndPage,
@@ -11,10 +16,10 @@ import {
   isFetchChangesetsPending,
   selectListAsCollection
 } from "../modules/changesets";
-import {connect} from "react-redux";
+import { connect } from "react-redux";
 import ChangesetList from "../components/changesets/ChangesetList";
-import {ErrorPage, LinkPaginator, Loading} from "@scm-manager/ui-components";
-import {translate} from "react-i18next";
+import { ErrorPage, LinkPaginator, Loading } from "@scm-manager/ui-components";
+import { translate } from "react-i18next";
 
 type Props = {
   repository: Repository, //TODO: Do we really need/want this here?
@@ -39,7 +44,6 @@ type State = {};
 
 class Changesets extends React.Component<Props, State> {
   componentDidMount() {
-    console.log("CDM");
     const {
       fetchChangesetsByBranch,
       fetchChangesetsByBranchAndPage,

scm-ui/src/repos/containers/RepositoryRoot.js

@@ -1,16 +1,29 @@
 //@flow
 import React from "react";
-import {deleteRepo, fetchRepo, getFetchRepoFailure, getRepository, isFetchRepoPending} from "../modules/repos";
-import {connect} from "react-redux";
-import {Route, Switch} from "react-router-dom";
-import type {Repository} from "@scm-manager/ui-types";
-import {ErrorPage, Loading, Navigation, NavLink, Page, Section} from "@scm-manager/ui-components";
-import {translate} from "react-i18next";
+import {
+  deleteRepo,
+  fetchRepo,
+  getFetchRepoFailure,
+  getRepository,
+  isFetchRepoPending
+} from "../modules/repos";
+import { connect } from "react-redux";
+import { Route, Switch } from "react-router-dom";
+import type { Repository } from "@scm-manager/ui-types";
+import {
+  ErrorPage,
+  Loading,
+  Navigation,
+  NavLink,
+  Page,
+  Section
+} from "@scm-manager/ui-components";
+import { translate } from "react-i18next";
 import RepositoryDetails from "../components/RepositoryDetails";
 import DeleteNavAction from "../components/DeleteNavAction";
 import Edit from "../containers/Edit";
 
-import type {History} from "history";
+import type { History } from "history";
 import EditNavLink from "../components/EditNavLink";
 import BranchRoot from "./BranchRoot";
 
@@ -98,9 +111,8 @@ class RepositoryRoot extends React.Component<Props> {
                 path={`${url}/edit`}
                 component={() => <Edit repository={repository} />}
               />
-
               <Route
-                path={`${url}/branches/:branch`}
+                path={`${url}/branches/:branch/changesets`}
                 render={() => (
                   <BranchRoot
                     repository={repository}
@@ -116,7 +128,7 @@ class RepositoryRoot extends React.Component<Props> {
                 <NavLink to={url} label={t("repository-root.information")} />
                 <NavLink
                   activeOnlyWhenExact={false}
-                  to={`${url}/changesets/`}
+                  to={`${url}/branches/master/changesets/1`}
                   label={t("repository-root.history")}
                   activeWhenMatch={this.matches}
                 />

scm-ui/src/repos/modules/branches.js

@@ -66,79 +66,51 @@ export function fetchBranchesFailure(repository: Repository, error: Error) {
 
 // Reducers
 
+type State = { [string]: Branch[] };
+
 export default function reducer(
-  state: Object = {},
+  state: State = {},
   action: Action = { type: "UNKNOWN" }
-): Object {
+): State {
   switch (action.type) {
     case FETCH_BRANCHES_SUCCESS:
       const key = createKey(action.payload.repository);
-      let oldBranchesByNames = { [key]: {} };
-      if (state[key] !== undefined) {
-        oldBranchesByNames[key] = state[key];
-      }
       return {
-        [key]: {
-          byNames: extractBranchesByNames(
-            action.payload.data,
-            oldBranchesByNames[key].byNames
-          )
-        }
+        ...state,
+        [key]: extractBranchesFromPayload(action.payload.data)
       };
     default:
       return state;
   }
 }
 
-function extractBranchesByNames(data: any, oldBranchesByNames: any): ?Object {
-  if (!data._embedded || !data._embedded.branches) {
-    return {};
+function extractBranchesFromPayload(payload: any) {
+  if (payload._embedded && payload._embedded.branches) {
+    return payload._embedded.branches;
   }
-  const branches = data._embedded.branches;
-  const branchesByNames = {};
-
-  for (let branch of branches) {
-    branchesByNames[branch.name] = branch;
-  }
-
-  for (let name in oldBranchesByNames) {
-    branchesByNames[name] = oldBranchesByNames[name];
-  }
-  return branchesByNames;
+  return [];
 }
 
 // Selectors
 
-export function getBranchNames(
-  state: Object,
-  repository: Repository
-): ?Array<Branch> {
-  const key = createKey(repository);
-  if (!state.branches || !state.branches[key] || !state.branches[key].byNames) {
-    return [];
-  }
-  return Object.keys(state.branches[key].byNames);
-}
+const empty = [];
 
 export function getBranches(state: Object, repository: Repository) {
   const key = createKey(repository);
   if (state.branches[key]) {
-    if (state.branches[key].byNames) {
-      return Object.values(state.branches[key].byNames);
-    }
+    return state.branches[key];
   }
+  return empty;
 }
 
 export function getBranch(
-  state: Object,
+  state: State,
   repository: Repository,
   name: string
 ): ?Branch {
   const key = createKey(repository);
   if (state.branches[key]) {
-    if (state.branches[key].byNames[name]) {
-      return state.branches[key].byNames[name];
-    }
+    return state.branches[key].find((b: Branch) => b.name === name);
   }
   return null;
 }

scm-ui/src/repos/modules/branches.test.js

@@ -109,23 +109,20 @@ describe("branches", () => {
       const newState = reducer({}, action);
       expect(newState).toBeDefined();
       expect(newState[key]).toBeDefined();
-      expect(newState[key].byNames["branch1"]).toEqual(branch1);
-      expect(newState[key].byNames["branch2"]).toEqual(branch2);
+      expect(newState[key]).toContain(branch1);
+      expect(newState[key]).toContain(branch2);
     });
 
     it("should not delete existing branches from state", () => {
       const oldState = {
-        "foo/bar": {
-          byNames: {
-            branch3: branch3
-          }
-        }
+        "hitchhiker/heartOfGold": [branch3]
       };
 
       const newState = reducer(oldState, action);
-      expect(newState[key].byNames["branch1"]).toEqual(branch1);
-      expect(newState[key].byNames["branch2"]).toEqual(branch2);
-      expect(newState[key].byNames["branch3"]).toEqual(branch3);
+      expect(newState[key]).toContain(branch1);
+      expect(newState[key]).toContain(branch2);
+
+      expect(newState["hitchhiker/heartOfGold"]).toContain(branch3);
     });
   });
 
@@ -134,12 +131,7 @@ describe("branches", () => {
 
     const state = {
       branches: {
-        [key]: {
-          byNames: {
-            branch1,
-            branch2
-          }
-        }
+        [key]: [branch1, branch2]
       }
     };
 
@@ -153,13 +145,6 @@ describe("branches", () => {
       expect(isFetchBranchesPending(state, repository)).toBeTruthy();
     });
 
-    it("should return branches names", () => {
-      const names = getBranchNames(state, repository);
-      expect(names.length).toEqual(2);
-      expect(names).toContain("branch1");
-      expect(names).toContain("branch2");
-    });
-
     it("should return branches", () => {
       const branches = getBranches(state, repository);
       expect(branches.length).toEqual(2);
@@ -167,15 +152,28 @@ describe("branches", () => {
       expect(branches).toContain(branch2);
     });
 
+    it("should return always the same reference for branches", () => {
+      const one = getBranches(state, repository);
+      const two = getBranches(state, repository);
+      expect(one).toBe(two);
+    });
+
     it("should return single branch by name", () => {
       const branch = getBranch(state, repository, "branch1");
       expect(branch).toEqual(branch1);
     });
 
+    it("should return same reference for single branch by name", () => {
+      const one = getBranch(state, repository, "branch1");
+      const two = getBranch(state, repository, "branch1");
+      expect(one).toBe(two);
+    });
+
     it("should return undefined if branch does not exist", () => {
       const branch = getBranch(state, repository, "branch42");
-      expect(branch).toBeFalsy();
+      expect(branch).toBeUndefined();
     });
+
     it("should return error if fetching branches failed", () => {
       const state = {
         failure: {