Merge branch '24629' into 'master'

Highlight currently focused/viewed file in file tree

See merge request gitlab-org/gitlab!27937

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

@@ -133,6 +133,7 @@ export default {
       'toggleFileDiscussions',
       'toggleFileDiscussionWrappers',
       'toggleFullDiff',
+      'toggleActiveFileByHash',
     ]),
     handleToggleFile() {
       this.$emit('toggleFile');
@@ -149,6 +150,9 @@ export default {
         const selector = this.diffContentIDSelector;
         scrollToElement(document.querySelector(selector));
         window.location.hash = selector;
+        if (!this.viewDiffsFileByFile) {
+          this.toggleActiveFileByHash(this.diffFile.file_hash);
+        }
       }
     },
   },

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

@@ -3,9 +3,10 @@
  * This component is an iterative step towards refactoring and simplifying `vue_shared/components/file_row.vue`
  * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23720
  */
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import FileRow from '~/vue_shared/components/file_row.vue';
-import FileRowStats from './file_row_stats.vue';
 import ChangedFileIcon from '~/vue_shared/components/changed_file_icon.vue';
+import FileRowStats from './file_row_stats.vue';
 
 export default {
   name: 'DiffFileRow',
@@ -14,6 +15,7 @@ export default {
     FileRowStats,
     ChangedFileIcon,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     file: {
       type: Object,
@@ -28,11 +30,28 @@ export default {
       required: false,
       default: null,
     },
+    viewedFiles: {
+      type: Object,
+      required: false,
+      default: () => ({}),
+    },
   },
   computed: {
     showFileRowStats() {
       return !this.hideFileStats && this.file.type === 'blob';
     },
+    fileClasses() {
+      if (!this.glFeatures.highlightCurrentDiffRow) {
+        return '';
+      }
+
+      return this.file.type === 'blob' && !this.viewedFiles[this.file.fileHash]
+        ? 'gl-font-weight-bold'
+        : '';
+    },
+    isActive() {
+      return this.currentDiffFileId === this.file.fileHash;
+    },
   },
 };
 </script>
@@ -41,8 +60,9 @@ export default {
   <file-row
     :file="file"
     v-bind="$attrs"
-    :class="{ 'is-active': currentDiffFileId === file.fileHash }"
+    :class="{ 'is-active': isActive }"
     class="diff-file-row"
+    :file-classes="fileClasses"
     v-on="$listeners"
   >
     <file-row-stats v-if="showFileRowStats" :file="file" class="mr-1" />

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

@@ -25,7 +25,7 @@ export default {
     };
   },
   computed: {
-    ...mapState('diffs', ['tree', 'renderTreeList', 'currentDiffFileId']),
+    ...mapState('diffs', ['tree', 'renderTreeList', 'currentDiffFileId', 'viewedDiffFileIds']),
     ...mapGetters('diffs', ['allBlobs']),
     filteredTreeList() {
       const search = this.search.toLowerCase().trim();
@@ -93,6 +93,7 @@ export default {
           :key="file.key"
           :file="file"
           :level="0"
+          :viewed-files="viewedDiffFileIds"
           :hide-file-stats="hideFileStats"
           :file-row-component="$options.DiffFileRow"
           :current-diff-file-id="currentDiffFileId"

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

@@ -84,7 +84,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
         commit(types.SET_BATCH_LOADING, false);
 
         if (!isNoteLink && !state.currentDiffFileId) {
-          commit(types.UPDATE_CURRENT_DIFF_FILE_ID, diff_files[0].file_hash);
+          commit(types.VIEW_DIFF_FILE, diff_files[0].file_hash);
         }
 
         if (isNoteLink) {
@@ -100,7 +100,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
             !state.diffFiles.some(f => f.file_hash === state.currentDiffFileId) &&
             !isNoteLink
           ) {
-            commit(types.UPDATE_CURRENT_DIFF_FILE_ID, state.diffFiles[0].file_hash);
+            commit(types.VIEW_DIFF_FILE, state.diffFiles[0].file_hash);
           }
 
           if (gon.features?.codeNavigation) {
@@ -183,7 +183,7 @@ export const fetchCoverageFiles = ({ commit, state }) => {
 export const setHighlightedRow = ({ commit }, lineCode) => {
   const fileHash = lineCode.split('_')[0];
   commit(types.SET_HIGHLIGHTED_ROW, lineCode);
-  commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
+  commit(types.VIEW_DIFF_FILE, fileHash);
 };
 
 // This is adding line discussions to the actual lines in the diff tree
@@ -428,13 +428,17 @@ export const toggleTreeOpen = ({ commit }, path) => {
   commit(types.TOGGLE_FOLDER_OPEN, path);
 };
 
+export const toggleActiveFileByHash = ({ commit }, hash) => {
+  commit(types.VIEW_DIFF_FILE, hash);
+};
+
 export const scrollToFile = ({ state, commit }, path) => {
   if (!state.treeEntries[path]) return;
 
   const { fileHash } = state.treeEntries[path];
   document.location.hash = fileHash;
 
-  commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
+  commit(types.VIEW_DIFF_FILE, fileHash);
 };
 
 export const toggleShowTreeList = ({ commit, state }, saving = true) => {
@@ -702,7 +706,7 @@ export const setCurrentDiffFileIdFromNote = ({ commit, state, rootGetters }, not
   const fileHash = rootGetters.getDiscussion(note.discussion_id).diff_file?.file_hash;
 
   if (fileHash && state.diffFiles.some(f => f.file_hash === fileHash)) {
-    commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
+    commit(types.VIEW_DIFF_FILE, fileHash);
   }
 };
 
@@ -710,5 +714,5 @@ export const navigateToDiffFileIndex = ({ commit, state }, index) => {
   const fileHash = state.diffFiles[index].file_hash;
   document.location.hash = fileHash;
 
-  commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
+  commit(types.VIEW_DIFF_FILE, fileHash);
 };

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -34,6 +34,7 @@ export default () => ({
   showTreeList: true,
   currentDiffFileId: '',
   projectPath: '',
+  viewedDiffFileIds: {},
   commentForms: [],
   highlightedRow: null,
   renderTreeList: true,

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

@@ -19,7 +19,7 @@ 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 UPDATE_CURRENT_DIFF_FILE_ID = 'UPDATE_CURRENT_DIFF_FILE_ID';
+export const VIEW_DIFF_FILE = 'VIEW_DIFF_FILE';
 
 export const OPEN_DIFF_FILE_COMMENT_FORM = 'OPEN_DIFF_FILE_COMMENT_FORM';
 export const UPDATE_DIFF_FILE_COMMENT_FORM = 'UPDATE_DIFF_FILE_COMMENT_FORM';

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

+import Vue from 'vue';
 import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { PARALLEL_DIFF_VIEW_TYPE } from '../constants';
 import {
@@ -291,8 +292,9 @@ export default {
   [types.TOGGLE_SHOW_TREE_LIST](state) {
     state.showTreeList = !state.showTreeList;
   },
-  [types.UPDATE_CURRENT_DIFF_FILE_ID](state, fileId) {
+  [types.VIEW_DIFF_FILE](state, fileId) {
     state.currentDiffFileId = fileId;
+    Vue.set(state.viewedDiffFileIds, fileId, true);
   },
   [types.OPEN_DIFF_FILE_COMMENT_FORM](state, formData) {
     state.commentForms.push({

app/assets/javascripts/vue_shared/components/file_row.vue

@@ -18,6 +18,11 @@ export default {
       type: Number,
       required: true,
     },
+    fileClasses: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   computed: {
     isTree() {
@@ -123,6 +128,7 @@ export default {
         :style="levelIndentation"
         class="file-row-name str-truncated"
         data-qa-selector="file_name_content"
+        :class="fileClasses"
       >
         <file-icon
           class="file-row-icon"

app/controllers/projects/merge_requests_controller.rb

@@ -39,6 +39,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:hide_jump_to_next_unresolved_in_threads, default_enabled: true)
     push_frontend_feature_flag(:merge_request_widget_graphql, @project)
     push_frontend_feature_flag(:unified_diff_lines, @project)
+    push_frontend_feature_flag(:highlight_current_diff_row, @project)
   end
 
   before_action do

changelogs/unreleased/24629.yml

+---
+title: Highlight un-focused/un-viewed file's in file tree
+merge_request: 27937
+author:
+type: changed

spec/frontend/diffs/components/diff_file_header_spec.js

@@ -44,6 +44,7 @@ describe('DiffFileHeader component', () => {
           toggleFileDiscussions: jest.fn(),
           toggleFileDiscussionWrappers: jest.fn(),
           toggleFullDiff: jest.fn(),
+          toggleActiveFileByHash: jest.fn(),
         },
       },
     },

spec/frontend/diffs/components/diff_file_row_spec.js

@@ -7,9 +7,12 @@ import ChangedFileIcon from '~/vue_shared/components/changed_file_icon.vue';
 describe('Diff File Row component', () => {
   let wrapper;
 
-  const createComponent = (props = {}) => {
+  const createComponent = (props = {}, highlightCurrentDiffRow = false) => {
     wrapper = shallowMount(DiffFileRow, {
       propsData: { ...props },
+      provide: {
+        glFeatures: { highlightCurrentDiffRow },
+      },
     });
   };
 
@@ -56,6 +59,31 @@ describe('Diff File Row component', () => {
     );
   });
 
+  it.each`
+    features                             | fileType  | isViewed | expected
+    ${{ highlightCurrentDiffRow: true }} | ${'blob'} | ${false} | ${'gl-font-weight-bold'}
+    ${{}}                                | ${'blob'} | ${true}  | ${''}
+    ${{}}                                | ${'tree'} | ${false} | ${''}
+    ${{}}                                | ${'tree'} | ${true}  | ${''}
+  `(
+    'with (features="$features", fileType="$fileType", isViewed=$isViewed), sets fileClasses="$expected"',
+    ({ features, fileType, isViewed, expected }) => {
+      createComponent(
+        {
+          file: {
+            type: fileType,
+            fileHash: '#123456789',
+          },
+          level: 0,
+          hideFileStats: false,
+          viewedFiles: isViewed ? { '#123456789': true } : {},
+        },
+        features.highlightCurrentDiffRow,
+      );
+      expect(wrapper.find(FileRow).props('fileClasses')).toBe(expected);
+    },
+  );
+
   describe('FileRowStats components', () => {
     it.each`
       type      | hideFileStats | value    | desc

spec/frontend/diffs/components/tree_list_spec.js

 import Vuex from 'vuex';
-import { mount, createLocalVue } from '@vue/test-utils';
+import { shallowMount, mount, createLocalVue } from '@vue/test-utils';
 import TreeList from '~/diffs/components/tree_list.vue';
 import createStore from '~/diffs/store/modules';
+import FileTree from '~/vue_shared/components/file_tree.vue';
 
 describe('Diffs tree list component', () => {
   let wrapper;
+  let store;
   const getFileRows = () => wrapper.findAll('.file-row');
   const localVue = createLocalVue();
   localVue.use(Vuex);
 
-  const createComponent = state => {
-    const store = new Vuex.Store({
+  const createComponent = (mountFn = mount) => {
+    wrapper = mountFn(TreeList, {
+      store,
+      localVue,
+      propsData: { hideFileStats: false },
+    });
+  };
+
+  beforeEach(() => {
+    store = new Vuex.Store({
       modules: {
         diffs: createStore(),
       },
@@ -23,61 +33,57 @@ describe('Diffs tree list component', () => {
       addedLines: 10,
       removedLines: 20,
       ...store.state.diffs,
-      ...state,
     };
+  });
 
-    wrapper = mount(TreeList, {
-      store,
-      localVue,
-      propsData: { hideFileStats: false },
+  const setupFilesInState = () => {
+    const treeEntries = {
+      'index.js': {
+        addedLines: 0,
+        changed: true,
+        deleted: false,
+        fileHash: 'test',
+        key: 'index.js',
+        name: 'index.js',
+        path: 'app/index.js',
+        removedLines: 0,
+        tempFile: true,
+        type: 'blob',
+        parentPath: 'app',
+      },
+      app: {
+        key: 'app',
+        path: 'app',
+        name: 'app',
+        type: 'tree',
+        tree: [],
+      },
+    };
+
+    Object.assign(store.state.diffs, {
+      treeEntries,
+      tree: [treeEntries['index.js'], treeEntries.app],
     });
   };
 
-  beforeEach(() => {
-    localStorage.removeItem('mr_diff_tree_list');
-
-    createComponent();
-  });
-
   afterEach(() => {
     wrapper.destroy();
   });
 
-  it('renders empty text', () => {
-    expect(wrapper.text()).toContain('No files found');
+  describe('default', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('renders empty text', () => {
+      expect(wrapper.text()).toContain('No files found');
+    });
   });
 
   describe('with files', () => {
     beforeEach(() => {
-      const treeEntries = {
-        'index.js': {
-          addedLines: 0,
-          changed: true,
-          deleted: false,
-          fileHash: 'test',
-          key: 'index.js',
-          name: 'index.js',
-          path: 'app/index.js',
-          removedLines: 0,
-          tempFile: true,
-          type: 'blob',
-          parentPath: 'app',
-        },
-        app: {
-          key: 'app',
-          path: 'app',
-          name: 'app',
-          type: 'tree',
-          tree: [],
-        },
-      };
-
-      createComponent({
-        treeEntries,
-        tree: [treeEntries['index.js'], treeEntries.app],
-      });
-
-      return wrapper.vm.$nextTick();
+      setupFilesInState();
+      createComponent();
     });
 
     it('renders tree', () => {
@@ -136,4 +142,23 @@ describe('Diffs tree list component', () => {
       });
     });
   });
+
+  describe('with viewedDiffFileIds', () => {
+    const viewedDiffFileIds = { fileId: '#12345' };
+
+    beforeEach(() => {
+      setupFilesInState();
+      store.state.diffs.viewedDiffFileIds = viewedDiffFileIds;
+    });
+
+    it('passes the viewedDiffFileIds to the FileTree', () => {
+      createComponent(shallowMount);
+
+      return wrapper.vm.$nextTick().then(() => {
+        // Have to use $attrs['viewed-files'] because we are passing down an object
+        // and attributes('') stringifies values (e.g. [object])...
+        expect(wrapper.find(FileTree).vm.$attrs['viewed-files']).toBe(viewedDiffFileIds);
+      });
+    });
+  });
 });

spec/frontend/diffs/store/actions_spec.js

@@ -191,10 +191,10 @@ describe('DiffsStoreActions', () => {
           { type: types.SET_RETRIEVING_BATCHES, payload: true },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } },
           { type: types.SET_BATCH_LOADING, payload: false },
-          { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'test' },
+          { type: types.VIEW_DIFF_FILE, payload: 'test' },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res2.diff_files } },
           { type: types.SET_BATCH_LOADING, payload: false },
-          { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'test2' },
+          { type: types.VIEW_DIFF_FILE, payload: 'test2' },
           { type: types.SET_RETRIEVING_BATCHES, payload: false },
         ],
         [],
@@ -300,7 +300,7 @@ describe('DiffsStoreActions', () => {
     it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => {
       testAction(setHighlightedRow, 'ABC_123', {}, [
         { type: types.SET_HIGHLIGHTED_ROW, payload: 'ABC_123' },
-        { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'ABC' },
+        { type: types.VIEW_DIFF_FILE, payload: 'ABC' },
       ]);
     });
   });
@@ -904,7 +904,7 @@ describe('DiffsStoreActions', () => {
       expect(document.location.hash).toBe('#test');
     });
 
-    it('commits UPDATE_CURRENT_DIFF_FILE_ID', () => {
+    it('commits VIEW_DIFF_FILE', () => {
       const state = {
         treeEntries: {
           path: {
@@ -915,7 +915,7 @@ describe('DiffsStoreActions', () => {
 
       scrollToFile({ state, commit }, 'path');
 
-      expect(commit).toHaveBeenCalledWith(types.UPDATE_CURRENT_DIFF_FILE_ID, 'test');
+      expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, 'test');
     });
   });
 
@@ -1413,7 +1413,7 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('setCurrentDiffFileIdFromNote', () => {
-    it('commits UPDATE_CURRENT_DIFF_FILE_ID', () => {
+    it('commits VIEW_DIFF_FILE', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1423,10 +1423,10 @@ describe('DiffsStoreActions', () => {
 
       setCurrentDiffFileIdFromNote({ commit, state, rootGetters }, '1');
 
-      expect(commit).toHaveBeenCalledWith(types.UPDATE_CURRENT_DIFF_FILE_ID, '123');
+      expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, '123');
     });
 
-    it('does not commit UPDATE_CURRENT_DIFF_FILE_ID when discussion has no diff_file', () => {
+    it('does not commit VIEW_DIFF_FILE when discussion has no diff_file', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1439,7 +1439,7 @@ describe('DiffsStoreActions', () => {
       expect(commit).not.toHaveBeenCalled();
     });
 
-    it('does not commit UPDATE_CURRENT_DIFF_FILE_ID when diff file does not exist', () => {
+    it('does not commit VIEW_DIFF_FILE when diff file does not exist', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1454,12 +1454,12 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('navigateToDiffFileIndex', () => {
-    it('commits UPDATE_CURRENT_DIFF_FILE_ID', done => {
+    it('commits VIEW_DIFF_FILE', done => {
       testAction(
         navigateToDiffFileIndex,
         0,
         { diffFiles: [{ file_hash: '123' }] },
-        [{ type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: '123' }],
+        [{ type: types.VIEW_DIFF_FILE, payload: '123' }],
         [],
         done,
       );

spec/frontend/diffs/store/mutations_spec.js

@@ -737,11 +737,11 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
-  describe('UPDATE_CURRENT_DIFF_FILE_ID', () => {
+  describe('VIEW_DIFF_FILE', () => {
     it('updates currentDiffFileId', () => {
       const state = createState();
 
-      mutations[types.UPDATE_CURRENT_DIFF_FILE_ID](state, 'somefileid');
+      mutations[types.VIEW_DIFF_FILE](state, 'somefileid');
 
       expect(state.currentDiffFileId).toBe('somefileid');
     });

spec/frontend/vue_shared/components/file_row_spec.js

@@ -139,4 +139,16 @@ describe('File row component', () => {
 
     expect(wrapper.vm.hasUrlAtCurrentRoute()).toBe(true);
   });
+
+  it('render with the correct file classes prop', () => {
+    createComponent({
+      file: {
+        ...file(),
+      },
+      level: 0,
+      fileClasses: 'font-weight-bold',
+    });
+
+    expect(wrapper.find('.file-row-name').classes()).toContain('font-weight-bold');
+  });
 });