[Code] file tree bug fix and more test (#41730)

* fix(code/frontend): fix file tree folder collapse * fix(code/frontend): fix flaky test; add more file tree test
elastic · Jul 25, 2019 · 5a572df · 5a572df
1 parent 5d9ef2a
commit 5a572df
Show file tree

Hide file tree

Showing 11 changed files with 232 additions and 101 deletions.
diff --git a/x-pack/legacy/plugins/code/public/actions/file.ts b/x-pack/legacy/plugins/code/public/actions/file.ts
@@ -58,10 +58,6 @@ export const fetchRepoTree = createAction<FetchRepoTreePayload>('FETCH REPO TREE
 export const fetchRepoTreeSuccess = createAction<RepoTreePayload>('FETCH REPO TREE SUCCESS');
 export const fetchRepoTreeFailed = createAction<Error>('FETCH REPO TREE FAILED');
 
-export const resetRepoTree = createAction('CLEAR REPO TREE');
-export const closeTreePath = createAction<string>('CLOSE TREE PATH');
-export const openTreePath = createAction<string>('OPEN TREE PATH');
-
 export const fetchRepoBranches = createAction<FetchRepoPayload>('FETCH REPO BRANCHES');
 export const fetchRepoBranchesSuccess = createAction<ReferenceInfo[]>(
   'FETCH REPO BRANCHES SUCCESS'

diff --git a/x-pack/legacy/plugins/code/public/components/file_tree/file_tree.test.tsx b/x-pack/legacy/plugins/code/public/components/file_tree/file_tree.test.tsx
@@ -9,7 +9,7 @@ import React from 'react';
 import { match } from 'react-router-dom';
 import renderer from 'react-test-renderer';
 import { MainRouteParams, PathTypes } from '../../common/types';
-import { createHistory, createLocation, createMatch, mockFunction } from '../../utils/test_utils';
+import { createHistory, createLocation, createMatch } from '../../utils/test_utils';
 import props from './__fixtures__/props.json';
 import { CodeFileTree } from './file_tree';
 
@@ -38,12 +38,9 @@ test('render correctly', () => {
     .create(
       <CodeFileTree
         node={props.node}
-        openedPaths={props.openedPaths}
         history={history}
         match={m}
         location={location}
-        closeTreePath={mockFunction}
-        openTreePath={mockFunction}
         isNotFound={false}
       />
     )

diff --git a/x-pack/legacy/plugins/code/public/components/file_tree/file_tree.tsx b/x-pack/legacy/plugins/code/public/components/file_tree/file_tree.tsx
@@ -11,28 +11,55 @@ import classes from 'classnames';
 import { connect } from 'react-redux';
 import { RouteComponentProps, withRouter } from 'react-router-dom';
 import { FileTree as Tree, FileTreeItemType } from '../../../model';
-import { closeTreePath, openTreePath } from '../../actions';
 import { EuiSideNavItem, MainRouteParams, PathTypes } from '../../common/types';
 import { RootState } from '../../reducers';
 import { encodeRevisionString } from '../../../common/uri_util';
 
 interface Props extends RouteComponentProps<MainRouteParams> {
   node?: Tree;
-  closeTreePath: (paths: string) => void;
-  openTreePath: (paths: string) => void;
-  openedPaths: string[];
   isNotFound: boolean;
 }
 
-export class CodeFileTree extends React.Component<Props> {
+export class CodeFileTree extends React.Component<Props, { openPaths: string[] }> {
   constructor(props: Props) {
     super(props);
     const { path } = props.match.params;
     if (path) {
-      props.openTreePath(path);
+      this.state = {
+        openPaths: CodeFileTree.getOpenPaths(path, []),
+      };
+    } else {
+      this.state = {
+        openPaths: [],
+      };
     }
   }
 
+  static getOpenPaths = (path: string, openPaths: string[]) => {
+    let p = path;
+    const newOpenPaths = [...openPaths];
+    const pathSegs = p.split('/');
+    while (!openPaths.includes(p)) {
+      newOpenPaths.push(p);
+      pathSegs.pop();
+      if (pathSegs.length <= 0) {
+        break;
+      }
+      p = pathSegs.join('/');
+    }
+    return newOpenPaths;
+  };
+
+  openTreePath = (path: string) => {
+    this.setState({ openPaths: CodeFileTree.getOpenPaths(path, this.state.openPaths) });
+  };
+
+  closeTreePath = (path: string) => {
+    const isSubFolder = (p: string) => p.startsWith(path + '/');
+    const newOpenPaths = this.state.openPaths.filter(p => !(p === path || isSubFolder(p)));
+    this.setState({ openPaths: newOpenPaths });
+  };
+
   public onClick = (node: Tree) => {
     const { resource, org, repo, revision, path } = this.props.match.params;
     if (!(path === node.path)) {
@@ -50,9 +77,9 @@ export class CodeFileTree extends React.Component<Props> {
 
   public toggleTree = (path: string) => {
     if (this.isPathOpen(path)) {
-      this.props.closeTreePath(path);
+      this.closeTreePath(path);
     } else {
-      this.props.openTreePath(path);
+      this.openTreePath(path);
     }
   };
 
@@ -241,24 +268,13 @@ export class CodeFileTree extends React.Component<Props> {
 
   private isPathOpen(path: string) {
     if (this.props.isNotFound) return false;
-    return this.props.openedPaths.includes(path);
+    return this.state.openPaths.includes(path);
   }
 }
 
 const mapStateToProps = (state: RootState) => ({
   node: state.fileTree.tree,
-  openedPaths: state.fileTree.openedPaths,
   isNotFound: state.file.isNotFound,
 });
 
-const mapDispatchToProps = {
-  closeTreePath,
-  openTreePath,
-};
-
-export const FileTree = withRouter(
-  connect(
-    mapStateToProps,
-    mapDispatchToProps
-  )(CodeFileTree)
-);
+export const FileTree = withRouter(connect(mapStateToProps)(CodeFileTree));
diff --git a/x-pack/legacy/plugins/code/public/reducers/file_tree.ts b/x-pack/legacy/plugins/code/public/reducers/file_tree.ts
@@ -8,13 +8,10 @@ import produce from 'immer';
 import { Action, handleActions } from 'redux-actions';
 import { FileTree, FileTreeItemType, sortFileTree } from '../../model';
 import {
-  closeTreePath,
   fetchRepoTree,
   fetchRepoTreeFailed,
   fetchRepoTreeSuccess,
-  openTreePath,
   RepoTreePayload,
-  resetRepoTree,
   fetchRootRepoTreeSuccess,
   fetchRootRepoTreeFailed,
   dirNotFound,
@@ -60,7 +57,6 @@ export function getPathOfTree(tree: FileTree, paths: string[]) {
 
 export interface FileTreeState {
   tree: FileTree;
-  openedPaths: string[];
   fileTreeLoadingPaths: string[];
   // store not found directory as an array to calculate `notFound` flag by finding whether path is in this array
   notFoundDirs: string[];
@@ -73,7 +69,6 @@ const initialState: FileTreeState = {
     path: '',
     type: FileTreeItemType.Directory,
   },
-  openedPaths: [],
   fileTreeLoadingPaths: [''],
   notFoundDirs: [],
   revision: '',
@@ -82,7 +77,6 @@ const initialState: FileTreeState = {
 const clearState = (state: FileTreeState) =>
   produce<FileTreeState>(state, draft => {
     draft.tree = initialState.tree;
-    draft.openedPaths = initialState.openedPaths;
     draft.fileTreeLoadingPaths = initialState.fileTreeLoadingPaths;
     draft.notFoundDirs = initialState.notFoundDirs;
     draft.revision = initialState.revision;
@@ -138,37 +132,12 @@ export const fileTree = handleActions<FileTreeState, FileTreePayload>(
       produce<FileTreeState>(state, draft => {
         draft.notFoundDirs.push(action.payload!);
       }),
-    [String(resetRepoTree)]: state =>
-      produce<FileTreeState>(state, draft => {
-        draft.tree = initialState.tree;
-        draft.openedPaths = initialState.openedPaths;
-      }),
     [String(fetchRepoTreeFailed)]: (state, action: Action<FileTree>) =>
       produce(state, draft => {
         draft.fileTreeLoadingPaths = draft.fileTreeLoadingPaths.filter(
           p => p !== action.payload!.path && p !== ''
         );
       }),
-    [String(openTreePath)]: (state, action: Action<string>) =>
-      produce<FileTreeState>(state, draft => {
-        let path = action.payload!;
-        const openedPaths = state.openedPaths;
-        const pathSegs = path.split('/');
-        while (!openedPaths.includes(path)) {
-          draft.openedPaths.push(path);
-          pathSegs.pop();
-          if (pathSegs.length <= 0) {
-            break;
-          }
-          path = pathSegs.join('/');
-        }
-      }),
-    [String(closeTreePath)]: (state, action: Action<string>) =>
-      produce<FileTreeState>(state, draft => {
-        const path = action.payload!;
-        const isSubFolder = (p: string) => p.startsWith(path + '/');
-        draft.openedPaths = state.openedPaths.filter(p => !(p === path || isSubFolder(p)));
-      }),
     [String(routePathChange)]: clearState,
     [String(repoChange)]: clearState,
     [String(revisionChange)]: clearState,

diff --git a/x-pack/legacy/plugins/code/public/sagas/editor.ts b/x-pack/legacy/plugins/code/public/sagas/editor.ts
@@ -16,19 +16,16 @@ import {
   closeReferences,
   fetchFile,
   FetchFileResponse,
-  fetchRepoCommits,
   fetchRepoTree,
   fetchTreeCommits,
   findReferences,
   findReferencesFailed,
   findReferencesSuccess,
   loadStructure,
   Match,
-  resetRepoTree,
   revealPosition,
   fetchRepos,
   turnOnDefaultRepoScope,
-  fetchRootRepoTree,
 } from '../actions';
 import { loadRepo, loadRepoFailed, loadRepoSuccess } from '../actions/status';
 import { PathTypes } from '../common/types';
@@ -42,7 +39,6 @@ import {
   repoScopeSelector,
   urlQueryStringSelector,
   createTreeSelector,
-  getTreeRevision,
   reposSelector,
 } from '../selectors';
 import { history } from '../utils/url';
@@ -188,14 +184,6 @@ function* handleMainRouteChange(action: Action<Match>) {
     }
   }
   const lastRequestPath = yield select(lastRequestPathSelector);
-  const currentTree: FileTree = yield select(getTree);
-  const currentTreeRevision: string = yield select(getTreeRevision);
-  // repo changed
-  if (currentTree.repoUri !== repoUri || revision !== currentTreeRevision) {
-    yield put(resetRepoTree());
-    yield put(fetchRepoCommits({ uri: repoUri, revision }));
-    yield put(fetchRootRepoTree({ uri: repoUri, revision }));
-  }
   const tree = yield select(getTree);
   const isDir = pathType === PathTypes.tree;
   function isTreeLoaded(isDirectory: boolean, targetTree: FileTree | null) {

diff --git a/x-pack/legacy/plugins/code/public/sagas/index.ts b/x-pack/legacy/plugins/code/public/sagas/index.ts
@@ -49,10 +49,11 @@ import {
 import { watchRootRoute } from './setup';
 import { watchRepoCloneSuccess, watchRepoDeleteFinished, watchStatusChange } from './status';
 import { watchLoadStructure } from './structure';
-import { watchRoute, watchRepoChange } from './route';
+import { watchRoute, watchRepoChange, watchRepoOrRevisionChange } from './route';
 
 export function* rootSaga() {
   yield fork(watchRepoChange);
+  yield fork(watchRepoOrRevisionChange);
   yield fork(watchRoute);
   yield fork(watchRootRoute);
   yield fork(watchLoadCommit);

diff --git a/x-pack/legacy/plugins/code/public/sagas/route.ts b/x-pack/legacy/plugins/code/public/sagas/route.ts
@@ -4,43 +4,65 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { put, takeEvery, select } from 'redux-saga/effects';
+import { put, takeEvery, select, takeLatest } from 'redux-saga/effects';
 import { Action } from 'redux-actions';
-import { routeChange, Match, loadRepo, fetchRepoBranches } from '../actions';
-import { previousMatchSelector } from '../selectors';
+import {
+  routeChange,
+  Match,
+  loadRepo,
+  fetchRepoBranches,
+  fetchRepoCommits,
+  fetchRootRepoTree,
+} from '../actions';
+import { previousMatchSelector, repoUriSelector, revisionSelector } from '../selectors';
 import { routePathChange, repoChange, revisionChange, filePathChange } from '../actions/route';
 import * as ROUTES from '../components/routes';
 
+const MAIN_ROUTES = [ROUTES.MAIN, ROUTES.MAIN_ROOT];
+
+function* handleRepoOrRevisionChange() {
+  const repoUri = yield select(repoUriSelector);
+  const revision = yield select(revisionSelector);
+  yield put(fetchRepoCommits({ uri: repoUri, revision }));
+  yield put(fetchRootRepoTree({ uri: repoUri, revision }));
+}
+
+export function* watchRepoOrRevisionChange() {
+  yield takeLatest([String(repoChange), String(revisionChange)], handleRepoOrRevisionChange);
+}
+
 const getRepoFromMatch = (match: Match) =>
   `${match.params.resource}/${match.params.org}/${match.params.repo}`;
 
 function* handleRoute(action: Action<any>) {
   const currentMatch = action.payload;
   const previousMatch = yield select(previousMatchSelector);
-  if (currentMatch.path !== previousMatch.path) {
-    yield put(routePathChange());
-    if (currentMatch.path === ROUTES.MAIN) {
+  if (MAIN_ROUTES.includes(currentMatch.path)) {
+    if (MAIN_ROUTES.includes(previousMatch.path)) {
+      const currentRepo = getRepoFromMatch(currentMatch);
+      const previousRepo = getRepoFromMatch(previousMatch);
+      const currentRevision = currentMatch.params.revision;
+      const previousRevision = previousMatch.params.revision;
+      const currentFilePath = currentMatch.params.path;
+      const previousFilePath = previousMatch.params.path;
+      if (currentRepo !== previousRepo) {
+        yield put(repoChange(currentRepo));
+      }
+      if (currentRevision !== previousRevision) {
+        yield put(revisionChange());
+      }
+      if (currentFilePath !== previousFilePath) {
+        yield put(filePathChange());
+      }
+    } else {
+      yield put(routePathChange());
       const currentRepo = getRepoFromMatch(currentMatch);
       yield put(repoChange(currentRepo));
       yield put(revisionChange());
       yield put(filePathChange());
     }
-  } else if (currentMatch.path === ROUTES.MAIN) {
-    const currentRepo = getRepoFromMatch(currentMatch);
-    const previousRepo = getRepoFromMatch(previousMatch);
-    const currentRevision = currentMatch.params.revision;
-    const previousRevision = previousMatch.params.revision;
-    const currentFilePath = currentMatch.params.path;
-    const previousFilePath = previousMatch.params.path;
-    if (currentRepo !== previousRepo) {
-      yield put(repoChange(currentRepo));
-    }
-    if (currentRevision !== previousRevision) {
-      yield put(revisionChange());
-    }
-    if (currentFilePath !== previousFilePath) {
-      yield put(filePathChange());
-    }
+  } else if (currentMatch.path !== previousMatch.path) {
+    yield put(routePathChange());
   }
 }
 

diff --git a/x-pack/legacy/plugins/code/public/selectors/index.ts b/x-pack/legacy/plugins/code/public/selectors/index.ts
@@ -35,6 +35,7 @@ export const repoUriSelector = (state: RootState) => {
   const { resource, org, repo } = state.route.match.params;
   return `${resource}/${org}/${repo}`;
 };
+export const revisionSelector = (state: RootState) => state.route.match.params.revision;
 
 export const routeSelector = (state: RootState) => state.route.match;
 

diff --git a/x-pack/test/functional/apps/code/explore_repository.ts b/x-pack/test/functional/apps/code/explore_repository.ts
@@ -23,8 +23,7 @@ export default function exploreRepositoryFunctionalTests({
 
   const FIND_TIME = config.get('timeouts.find');
 
-  // FLAKY: https://github.com/elastic/kibana/issues/41453
-  describe.skip('Explore Repository', function() {
+  describe('Explore Repository', function() {
     this.tags('smoke');
     describe('Explore a repository', () => {
       const repositoryListSelector = 'codeRepositoryList codeRepositoryItem';
@@ -121,8 +120,7 @@ export default function exploreRepositoryFunctionalTests({
         });
       });
 
-      // FLAKY: https://github.com/elastic/kibana/issues/41076
-      it.skip('Click file/directory on the file tree', async () => {
+      it('Click file/directory on the file tree', async () => {
         log.debug('Click a file in the source tree');
         // Wait the file tree to be rendered and click the 'src' folder on the file tree.
         await retry.try(async () => {
@@ -218,8 +216,7 @@ export default function exploreRepositoryFunctionalTests({
         });
       });
 
-      // FLAKY: https://github.com/elastic/kibana/issues/41112
-      it.skip('click a breadcrumb should not affect the file tree', async () => {
+      it('click a breadcrumb should not affect the file tree', async () => {
         log.debug('it goes to a deep node of file tree');
         const url = `${PageObjects.common.getHostPort()}/app/code#/github.com/elastic/TypeScript-Node-Starter/blob/master/src/models/User.ts`;
         await browser.get(url);