Merge branch 'ph/fixMRTreeListShowingIncorrectly' into 'master'

Fixes the diffs tree list showing incorrectly

See merge request gitlab-org/gitlab!48478

app/assets/javascripts/diffs/components/app.vue

 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
 import { GlLoadingIcon, GlPagination, GlSprintf } from '@gitlab/ui';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import Mousetrap from 'mousetrap';
 import { __ } from '~/locale';
 import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
@@ -316,7 +317,7 @@ export default {
       'setHighlightedRow',
       'cacheTreeListWidth',
       'scrollToFile',
-      'toggleShowTreeList',
+      'setShowTreeList',
       'navigateToDiffFileIndex',
     ]),
     navigateToDiffFileNumber(number) {
@@ -343,7 +344,7 @@ export default {
       this.fetchDiffFilesMeta()
         .then(({ real_size }) => {
           this.diffFilesLength = parseInt(real_size, 10);
-          if (toggleTree) this.hideTreeListIfJustOneFile();
+          if (toggleTree) this.setTreeDisplay();
 
           this.startDiffRendering();
         })
@@ -353,6 +354,7 @@ export default {
 
       this.fetchDiffFilesBatch()
         .then(() => {
+          if (toggleTree) this.setTreeDisplay();
           // Guarantee the discussions are assigned after the batch finishes.
           // Just watching the length of the discussions or the diff files
           // isn't enough, because with split diff loading, neither will
@@ -422,12 +424,17 @@ export default {
         this.scrollToFile(this.diffFiles[targetIndex].file_path);
       }
     },
-    hideTreeListIfJustOneFile() {
+    setTreeDisplay() {
       const storedTreeShow = localStorage.getItem(MR_TREE_SHOW_KEY);
+      let showTreeList = true;
 
-      if ((storedTreeShow === null && this.diffFiles.length <= 1) || storedTreeShow === 'false') {
-        this.toggleShowTreeList(false);
+      if (storedTreeShow !== null) {
+        showTreeList = Boolean(storedTreeShow);
+      } else if (!bp.isDesktop() || (!this.isBatchLoading && this.diffFiles.length <= 1)) {
+        showTreeList = false;
       }
+
+      return this.setShowTreeList({ showTreeList, saving: false });
     },
   },
   minTreeWidth: MIN_TREE_WIDTH,

app/assets/javascripts/diffs/components/compare_versions.vue

@@ -65,11 +65,7 @@ export default {
     polyfillSticky(this.$el);
   },
   methods: {
-    ...mapActions('diffs', [
-      'setInlineDiffViewType',
-      'setParallelDiffViewType',
-      'toggleShowTreeList',
-    ]),
+    ...mapActions('diffs', ['setInlineDiffViewType', 'setParallelDiffViewType', 'setShowTreeList']),
     expandAllFiles() {
       eventHub.$emit(EVT_EXPAND_ALL_FILES);
     },
@@ -92,7 +88,7 @@ export default {
         class="gl-mr-3 js-toggle-tree-list"
         :title="toggleFileBrowserTitle"
         :selected="showTreeList"
-        @click="toggleShowTreeList"
+        @click="setShowTreeList({ showTreeList: !showTreeList })"
       />
       <gl-sprintf
         v-if="showDropdowns"

app/assets/javascripts/diffs/store/actions.js

@@ -447,11 +447,11 @@ export const scrollToFile = ({ state, commit }, path) => {
   commit(types.VIEW_DIFF_FILE, fileHash);
 };
 
-export const toggleShowTreeList = ({ commit, state }, saving = true) => {
-  commit(types.TOGGLE_SHOW_TREE_LIST);
+export const setShowTreeList = ({ commit }, { showTreeList, saving = true }) => {
+  commit(types.SET_SHOW_TREE_LIST, showTreeList);
 
   if (saving) {
-    localStorage.setItem(MR_TREE_SHOW_KEY, state.showTreeList);
+    localStorage.setItem(MR_TREE_SHOW_KEY, showTreeList);
   }
 };
 

app/assets/javascripts/diffs/store/mutation_types.js

@@ -17,7 +17,7 @@ export const RENDER_FILE = 'RENDER_FILE';
 export const SET_LINE_DISCUSSIONS_FOR_FILE = 'SET_LINE_DISCUSSIONS_FOR_FILE';
 export const REMOVE_LINE_DISCUSSIONS_FOR_FILE = 'REMOVE_LINE_DISCUSSIONS_FOR_FILE';
 export const TOGGLE_FOLDER_OPEN = 'TOGGLE_FOLDER_OPEN';
-export const TOGGLE_SHOW_TREE_LIST = 'TOGGLE_SHOW_TREE_LIST';
+export const SET_SHOW_TREE_LIST = 'SET_SHOW_TREE_LIST';
 export const VIEW_DIFF_FILE = 'VIEW_DIFF_FILE';
 
 export const OPEN_DIFF_FILE_COMMENT_FORM = 'OPEN_DIFF_FILE_COMMENT_FORM';

app/assets/javascripts/diffs/store/mutations.js

@@ -247,8 +247,8 @@ export default {
   [types.TOGGLE_FOLDER_OPEN](state, path) {
     state.treeEntries[path].opened = !state.treeEntries[path].opened;
   },
-  [types.TOGGLE_SHOW_TREE_LIST](state) {
-    state.showTreeList = !state.showTreeList;
+  [types.SET_SHOW_TREE_LIST](state, showTreeList) {
+    state.showTreeList = showTreeList;
   },
   [types.VIEW_DIFF_FILE](state, fileId) {
     state.currentDiffFileId = fileId;

spec/frontend/diffs/components/app_spec.js

@@ -638,47 +638,47 @@ describe('diffs/components/app', () => {
     });
   });
 
-  describe('hideTreeListIfJustOneFile', () => {
-    let toggleShowTreeList;
+  describe('setTreeDisplay', () => {
+    let setShowTreeList;
 
     beforeEach(() => {
-      toggleShowTreeList = jest.fn();
+      setShowTreeList = jest.fn();
     });
 
     afterEach(() => {
       localStorage.removeItem('mr_tree_show');
     });
 
-    it('calls toggleShowTreeList when only 1 file', () => {
+    it('calls setShowTreeList when only 1 file', () => {
       createComponent({}, ({ state }) => {
         state.diffs.diffFiles.push({ sha: '123' });
       });
 
       wrapper.setMethods({
-        toggleShowTreeList,
+        setShowTreeList,
       });
 
-      wrapper.vm.hideTreeListIfJustOneFile();
+      wrapper.vm.setTreeDisplay();
 
-      expect(toggleShowTreeList).toHaveBeenCalledWith(false);
+      expect(setShowTreeList).toHaveBeenCalledWith({ showTreeList: false, saving: false });
     });
 
-    it('does not call toggleShowTreeList when more than 1 file', () => {
+    it('calls setShowTreeList with true when more than 1 file is in diffs array', () => {
       createComponent({}, ({ state }) => {
         state.diffs.diffFiles.push({ sha: '123' });
         state.diffs.diffFiles.push({ sha: '124' });
       });
 
       wrapper.setMethods({
-        toggleShowTreeList,
+        setShowTreeList,
       });
 
-      wrapper.vm.hideTreeListIfJustOneFile();
+      wrapper.vm.setTreeDisplay();
 
-      expect(toggleShowTreeList).not.toHaveBeenCalled();
+      expect(setShowTreeList).toHaveBeenCalledWith({ showTreeList: true, saving: false });
     });
 
-    it('does not call toggleShowTreeList when localStorage is set', () => {
+    it('calls setShowTreeList with localstorage value', () => {
       localStorage.setItem('mr_tree_show', 'true');
 
       createComponent({}, ({ state }) => {
@@ -686,12 +686,12 @@ describe('diffs/components/app', () => {
       });
 
       wrapper.setMethods({
-        toggleShowTreeList,
+        setShowTreeList,
       });
 
-      wrapper.vm.hideTreeListIfJustOneFile();
+      wrapper.vm.setTreeDisplay();
 
-      expect(toggleShowTreeList).not.toHaveBeenCalled();
+      expect(setShowTreeList).toHaveBeenCalledWith({ showTreeList: true, saving: false });
     });
   });
 

spec/frontend/diffs/store/actions_spec.js

@@ -32,7 +32,7 @@ import {
   setHighlightedRow,
   toggleTreeOpen,
   scrollToFile,
-  toggleShowTreeList,
+  setShowTreeList,
   renderFileForDiscussionId,
   setRenderTreeList,
   setShowWhitespace,
@@ -901,15 +901,22 @@ describe('DiffsStoreActions', () => {
     });
   });
 
-  describe('toggleShowTreeList', () => {
+  describe('setShowTreeList', () => {
     it('commits toggle', done => {
-      testAction(toggleShowTreeList, null, {}, [{ type: types.TOGGLE_SHOW_TREE_LIST }], [], done);
+      testAction(
+        setShowTreeList,
+        { showTreeList: true },
+        {},
+        [{ type: types.SET_SHOW_TREE_LIST, payload: true }],
+        [],
+        done,
+      );
     });
 
     it('updates localStorage', () => {
       jest.spyOn(localStorage, 'setItem').mockImplementation(() => {});
 
-      toggleShowTreeList({ commit() {}, state: { showTreeList: true } });
+      setShowTreeList({ commit() {} }, { showTreeList: true });
 
       expect(localStorage.setItem).toHaveBeenCalledWith('mr_tree_show', true);
     });
@@ -917,7 +924,7 @@ describe('DiffsStoreActions', () => {
     it('does not update localStorage', () => {
       jest.spyOn(localStorage, 'setItem').mockImplementation(() => {});
 
-      toggleShowTreeList({ commit() {}, state: { showTreeList: true } }, false);
+      setShowTreeList({ commit() {} }, { showTreeList: true, saving: false });
 
       expect(localStorage.setItem).not.toHaveBeenCalled();
     });

spec/frontend/diffs/store/mutations_spec.js

@@ -621,15 +621,11 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
-  describe('TOGGLE_SHOW_TREE_LIST', () => {
-    it('toggles showTreeList', () => {
+  describe('SET_SHOW_TREE_LIST', () => {
+    it('sets showTreeList', () => {
       const state = createState();
 
-      mutations[types.TOGGLE_SHOW_TREE_LIST](state);
-
-      expect(state.showTreeList).toBe(false, 'Failed to toggle showTreeList to false');
-
-      mutations[types.TOGGLE_SHOW_TREE_LIST](state);
+      mutations[types.SET_SHOW_TREE_LIST](state, true);
 
       expect(state.showTreeList).toBe(true, 'Failed to toggle showTreeList to true');
     });