Merge branch '273067-fix-ide-open-mr-highligh-first-file' into 'master'

Fix IDE openMergeRequest to route to first file

See merge request gitlab-org/gitlab!53927

app/assets/javascripts/ide/constants.js

@@ -107,3 +107,7 @@ export const SIDE_RIGHT = 'right';
 
 // Live Preview feature
 export const LIVE_PREVIEW_DEBOUNCE = 2000;
+
+// This is the maximum number of files to auto open when opening the Web IDE
+// from a Merge Request
+export const MAX_MR_FILES_AUTO_OPEN = 10;

app/assets/javascripts/ide/stores/actions/file.js

@@ -120,10 +120,6 @@ export const getFileData = (
     });
 };
 
-export const setFileMrChange = ({ commit }, { file, mrChange }) => {
-  commit(types.SET_FILE_MERGE_REQUEST_CHANGE, { file, mrChange });
-};
-
 export const getRawFileData = ({ state, commit, dispatch, getters }, { path }) => {
   const file = state.entries[path];
   const stagedFile = state.stagedFiles.find((f) => f.path === path);

app/assets/javascripts/ide/stores/actions/merge_request.js

 import { deprecatedCreateFlash as flash } from '~/flash';
 import { __ } from '~/locale';
-import { leftSidebarViews, PERMISSION_READ_MR } from '../../constants';
+import { leftSidebarViews, PERMISSION_READ_MR, MAX_MR_FILES_AUTO_OPEN } from '../../constants';
 import service from '../../services';
 import * as types from '../mutation_types';
 
@@ -147,70 +147,96 @@ export const getMergeRequestVersions = (
     }
   });
 
-export const openMergeRequest = (
-  { dispatch, state, getters },
-  { projectId, targetProjectId, mergeRequestId } = {},
-) =>
-  dispatch('getMergeRequestData', {
-    projectId,
-    targetProjectId,
-    mergeRequestId,
-  })
-    .then((mr) => {
-      dispatch('setCurrentBranchId', mr.source_branch);
-
-      return dispatch('getBranchData', {
-        projectId,
-        branchId: mr.source_branch,
-      }).then(() => {
-        const branch = getters.findBranch(projectId, mr.source_branch);
-
-        return dispatch('getFiles', {
-          projectId,
-          branchId: mr.source_branch,
-          ref: branch.commit.id,
+export const openMergeRequestChanges = async ({ dispatch, getters, state, commit }, changes) => {
+  const entryChanges = changes
+    .map((change) => ({ entry: state.entries[change.new_path], change }))
+    .filter((x) => x.entry);
+
+  const pathsToOpen = entryChanges
+    .slice(0, MAX_MR_FILES_AUTO_OPEN)
+    .map(({ change }) => change.new_path);
+
+  // If there are no changes with entries, do nothing.
+  if (!entryChanges.length) {
+    return;
+  }
+
+  dispatch('updateActivityBarView', leftSidebarViews.review.name);
+
+  entryChanges.forEach(({ change, entry }) => {
+    commit(types.SET_FILE_MERGE_REQUEST_CHANGE, { file: entry, mrChange: change });
+  });
+
+  // Open paths in order as they appear in MR changes
+  pathsToOpen.forEach((path) => {
+    commit(types.TOGGLE_FILE_OPEN, path);
+  });
+
+  // Activate first path.
+  // We don't `getFileData` here since the editor component kicks that off. Otherwise, we'd fetch twice.
+  const [firstPath, ...remainingPaths] = pathsToOpen;
+  await dispatch('router/push', getters.getUrlForPath(firstPath));
+  await dispatch('setFileActive', firstPath);
+
+  // Lastly, eagerly fetch the remaining paths for improved user experience.
+  await Promise.all(
+    remainingPaths.map(async (path) => {
+      try {
+        await dispatch('getFileData', {
+          path,
+          makeFileActive: false,
         });
-      });
-    })
-    .then(() =>
-      dispatch('getMergeRequestVersions', {
-        projectId,
-        targetProjectId,
-        mergeRequestId,
-      }),
-    )
-    .then(() =>
-      dispatch('getMergeRequestChanges', {
-        projectId,
-        targetProjectId,
-        mergeRequestId,
-      }),
-    )
-    .then((mrChanges) => {
-      if (mrChanges.changes.length) {
-        dispatch('updateActivityBarView', leftSidebarViews.review.name);
+        await dispatch('getRawFileData', { path });
+      } catch (e) {
+        // If one of the file fetches fails, we dont want to blow up the rest of them.
+        // eslint-disable-next-line no-console
+        console.error('[gitlab] An unexpected error occurred fetching MR file data', e);
       }
+    }),
+  );
+};
 
-      mrChanges.changes.forEach((change, ind) => {
-        const changeTreeEntry = state.entries[change.new_path];
+export const openMergeRequest = async (
+  { dispatch, getters },
+  { projectId, targetProjectId, mergeRequestId } = {},
+) => {
+  try {
+    const mr = await dispatch('getMergeRequestData', {
+      projectId,
+      targetProjectId,
+      mergeRequestId,
+    });
 
-        if (changeTreeEntry) {
-          dispatch('setFileMrChange', {
-            file: changeTreeEntry,
-            mrChange: change,
-          });
+    dispatch('setCurrentBranchId', mr.source_branch);
 
-          if (ind < 10) {
-            dispatch('getFileData', {
-              path: change.new_path,
-              makeFileActive: ind === 0,
-              openFile: true,
-            });
-          }
-        }
-      });
-    })
-    .catch((e) => {
-      flash(__('Error while loading the merge request. Please try again.'));
-      throw e;
+    await dispatch('getBranchData', {
+      projectId,
+      branchId: mr.source_branch,
+    });
+
+    const branch = getters.findBranch(projectId, mr.source_branch);
+
+    await dispatch('getFiles', {
+      projectId,
+      branchId: mr.source_branch,
+      ref: branch.commit.id,
     });
+
+    await dispatch('getMergeRequestVersions', {
+      projectId,
+      targetProjectId,
+      mergeRequestId,
+    });
+
+    const { changes } = await dispatch('getMergeRequestChanges', {
+      projectId,
+      targetProjectId,
+      mergeRequestId,
+    });
+
+    await dispatch('openMergeRequestChanges', changes);
+  } catch (e) {
+    flash(__('Error while loading the merge request. Please try again.'));
+    throw e;
+  }
+};

changelogs/unreleased/273067-fix-ide-open-mr-highligh-first-file.yml

+---
+title: Fix Web IDE open MR to show opened files consistently
+merge_request: 53927
+author:
+type: fixed

spec/frontend/fixtures/api_merge_requests.rb

@@ -6,9 +6,10 @@ RSpec.describe API::MergeRequests, '(JavaScript fixtures)', type: :request do
   include ApiHelpers
   include JavaScriptFixturesHelpers
 
-  let(:admin) { create(:admin, name: 'root') }
-  let(:namespace) { create(:namespace, name: 'gitlab-test' )}
-  let(:project) { create(:project, :repository, namespace: namespace, path: 'lorem-ipsum') }
+  let_it_be(:admin) { create(:admin, name: 'root') }
+  let_it_be(:namespace) { create(:namespace, name: 'gitlab-test' )}
+  let_it_be(:project) { create(:project, :repository, namespace: namespace, path: 'lorem-ipsum') }
+  let_it_be(:mr) { create(:merge_request, source_project: project) }
 
   before(:all) do
     clean_frontend_fixtures('api/merge_requests')
@@ -21,4 +22,16 @@ RSpec.describe API::MergeRequests, '(JavaScript fixtures)', type: :request do
 
     expect(response).to be_successful
   end
+
+  it 'api/merge_requests/versions.json' do
+    get api("/projects/#{project.id}/merge_requests/#{mr.iid}/versions", admin)
+
+    expect(response).to be_successful
+  end
+
+  it 'api/merge_requests/changes.json' do
+    get api("/projects/#{project.id}/merge_requests/#{mr.iid}/changes", admin)
+
+    expect(response).to be_successful
+  end
 end

spec/frontend/ide/stores/actions/merge_request_spec.js

 import MockAdapter from 'axios-mock-adapter';
+import { range } from 'lodash';
+import { TEST_HOST } from 'helpers/test_constants';
+import testAction from 'helpers/vuex_action_helper';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
-import { leftSidebarViews, PERMISSION_READ_MR } from '~/ide/constants';
+import { leftSidebarViews, PERMISSION_READ_MR, MAX_MR_FILES_AUTO_OPEN } from '~/ide/constants';
 import service from '~/ide/services';
 import { createStore } from '~/ide/stores';
 import {
   getMergeRequestData,
   getMergeRequestChanges,
   getMergeRequestVersions,
+  openMergeRequestChanges,
   openMergeRequest,
 } from '~/ide/stores/actions/merge_request';
+import * as types from '~/ide/stores/mutation_types';
 import axios from '~/lib/utils/axios_utils';
 
 const TEST_PROJECT = 'abcproject';
 const TEST_PROJECT_ID = 17;
 
+const createMergeRequestChange = (path) => ({
+  new_path: path,
+  path,
+});
+const createMergeRequestChangesCount = (n) =>
+  range(n).map((i) => createMergeRequestChange(`loremispum_${i}.md`));
+
+const testGetUrlForPath = (path) => `${TEST_HOST}/test/${path}`;
+
 jest.mock('~/flash');
 
 describe('IDE store merge request actions', () => {
@@ -353,6 +367,72 @@ describe('IDE store merge request actions', () => {
     });
   });
 
+  describe('openMergeRequestChanges', () => {
+    it.each`
+      desc                                   | changes                     | entries
+      ${'with empty changes'}                | ${[]}                       | ${{}}
+      ${'with changes not matching entries'} | ${[{ new_path: '123.md' }]} | ${{ '456.md': {} }}
+    `('$desc, does nothing', ({ changes, entries }) => {
+      const state = { entries };
+
+      return testAction({
+        action: openMergeRequestChanges,
+        state,
+        payload: changes,
+        expectedActions: [],
+        expectedMutations: [],
+      });
+    });
+
+    it('updates views and opens mr changes', () => {
+      // This is the payload sent to the action
+      const changesPayload = createMergeRequestChangesCount(15);
+
+      // Remove some items from the payload to use for entries
+      const changes = changesPayload.slice(1, 14);
+
+      const entries = changes.reduce(
+        (acc, { path }) => Object.assign(acc, { [path]: path, type: 'blob' }),
+        {},
+      );
+      const pathsToOpen = changes.slice(0, MAX_MR_FILES_AUTO_OPEN).map((x) => x.new_path);
+
+      return testAction({
+        action: openMergeRequestChanges,
+        state: { entries, getUrlForPath: testGetUrlForPath },
+        payload: changesPayload,
+        expectedActions: [
+          { type: 'updateActivityBarView', payload: leftSidebarViews.review.name },
+          // Only activates first file
+          { type: 'router/push', payload: testGetUrlForPath(pathsToOpen[0]) },
+          { type: 'setFileActive', payload: pathsToOpen[0] },
+          // Fetches data for other files
+          ...pathsToOpen.slice(1).map((path) => ({
+            type: 'getFileData',
+            payload: { path, makeFileActive: false },
+          })),
+          ...pathsToOpen.slice(1).map((path) => ({
+            type: 'getRawFileData',
+            payload: { path },
+          })),
+        ],
+        expectedMutations: [
+          ...changes.map((change) => ({
+            type: types.SET_FILE_MERGE_REQUEST_CHANGE,
+            payload: {
+              file: entries[change.new_path],
+              mrChange: change,
+            },
+          })),
+          ...pathsToOpen.map((path) => ({
+            type: types.TOGGLE_FILE_OPEN,
+            payload: path,
+          })),
+        ],
+      });
+    });
+  });
+
   describe('openMergeRequest', () => {
     const mr = {
       projectId: TEST_PROJECT,
@@ -409,7 +489,6 @@ describe('IDE store merge request actions', () => {
           case 'getFiles':
           case 'getMergeRequestVersions':
           case 'getBranchData':
-          case 'setFileMrChange':
             return Promise.resolve();
           default:
             return originalDispatch(type, payload);
@@ -445,6 +524,7 @@ describe('IDE store merge request actions', () => {
             ],
             ['getMergeRequestVersions', mr],
             ['getMergeRequestChanges', mr],
+            ['openMergeRequestChanges', testMergeRequestChanges.changes],
           ]);
         })
         .then(done)
@@ -454,9 +534,11 @@ describe('IDE store merge request actions', () => {
     it('updates activity bar view and gets file data, if changes are found', (done) => {
       store.state.entries.foo = {
         type: 'blob',
+        path: 'foo',
       };
       store.state.entries.bar = {
         type: 'blob',
+        path: 'bar',
       };
 
       testMergeRequestChanges.changes = [
@@ -467,24 +549,9 @@ describe('IDE store merge request actions', () => {
       openMergeRequest({ state: store.state, dispatch: store.dispatch, getters: mockGetters }, mr)
         .then(() => {
           expect(store.dispatch).toHaveBeenCalledWith(
-            'updateActivityBarView',
-            leftSidebarViews.review.name,
+            'openMergeRequestChanges',
+            testMergeRequestChanges.changes,
           );
-
-          testMergeRequestChanges.changes.forEach((change, i) => {
-            expect(store.dispatch).toHaveBeenCalledWith('setFileMrChange', {
-              file: store.state.entries[change.new_path],
-              mrChange: change,
-            });
-
-            expect(store.dispatch).toHaveBeenCalledWith('getFileData', {
-              path: change.new_path,
-              makeFileActive: i === 0,
-              openFile: true,
-            });
-          });
-
-          expect(store.state.openFiles.length).toBe(testMergeRequestChanges.changes.length);
         })
         .then(done)
         .catch(done.fail);

spec/frontend_integration/ide/helpers/ide_helper.js

@@ -69,7 +69,7 @@ const openFileRow = (row) => {
   row.click();
 };
 
-const findAndTraverseToPath = async (path, index = 0, row = null) => {
+export const findAndTraverseToPath = async (path, index = 0, row = null) => {
   if (!path) {
     return row;
   }
@@ -110,6 +110,12 @@ const findAndClickRootAction = async (name) => {
   button.click();
 };
 
+/**
+ * Drop leading "/-/ide" and file path from the current URL
+ */
+export const getBaseRoute = (url = window.location.pathname) =>
+  url.replace(/^\/-\/ide/, '').replace(/\/-\/.*$/, '');
+
 export const clickPreviewMarkdown = () => {
   screen.getByText('Preview Markdown').click();
 };

spec/frontend_integration/ide/helpers/start.js

@@ -4,11 +4,12 @@ import Editor from '~/ide/lib/editor';
 import extendStore from '~/ide/stores/extend';
 import { IDE_DATASET } from './mock_data';
 
-export default (container, { isRepoEmpty = false, path = '' } = {}) => {
+export default (container, { isRepoEmpty = false, path = '', mrId = '' } = {}) => {
+  const projectName = isRepoEmpty ? 'lorem-ipsum-empty' : 'lorem-ipsum';
+  const pathSuffix = mrId ? `merge_requests/${mrId}` : `tree/master/-/${path}`;
+
   global.jsdom.reconfigure({
-    url: `${TEST_HOST}/-/ide/project/gitlab-test/lorem-ipsum${
-      isRepoEmpty ? '-empty' : ''
-    }/tree/master/-/${path}`,
+    url: `${TEST_HOST}/-/ide/project/gitlab-test/${projectName}/${pathSuffix}`,
   });
 
   const el = document.createElement('div');

spec/frontend_integration/ide/user_opens_mr_spec.js

+import { basename } from 'path';
+import { getMergeRequests, getMergeRequestWithChanges } from 'test_helpers/fixtures';
+import { useOverclockTimers } from 'test_helpers/utils/overclock_timers';
+import * as ideHelper from './helpers/ide_helper';
+import startWebIDE from './helpers/start';
+
+const getRelevantChanges = () =>
+  getMergeRequestWithChanges().changes.filter((x) => !x.deleted_file);
+
+describe('IDE: User opens Merge Request', () => {
+  useOverclockTimers();
+
+  let vm;
+  let container;
+  let changes;
+
+  beforeEach(async () => {
+    const [{ iid: mrId }] = getMergeRequests();
+
+    changes = getRelevantChanges();
+
+    setFixtures('<div class="webide-container"></div>');
+    container = document.querySelector('.webide-container');
+
+    vm = startWebIDE(container, { mrId });
+
+    await ideHelper.waitForTabToOpen(basename(changes[0].new_path));
+    await ideHelper.waitForMonacoEditor();
+  });
+
+  afterEach(async () => {
+    vm.$destroy();
+    vm = null;
+  });
+
+  const findAllTabs = () => Array.from(document.querySelectorAll('.multi-file-tab'));
+  const findAllTabsData = () =>
+    findAllTabs().map((el) => ({
+      title: el.getAttribute('title'),
+      text: el.textContent.trim(),
+    }));
+
+  it('shows first change as active in file tree', async () => {
+    const firstPath = changes[0].new_path;
+    const row = await ideHelper.findAndTraverseToPath(firstPath);
+
+    expect(row).toHaveClass('is-open');
+    expect(row).toHaveClass('is-active');
+  });
+
+  it('opens other changes', () => {
+    // We only show first 10 changes
+    const expectedTabs = changes.slice(0, 10).map((x) => ({
+      title: `${ideHelper.getBaseRoute()}/-/${x.new_path}/`,
+      text: basename(x.new_path),
+    }));
+
+    expect(findAllTabsData()).toEqual(expectedTabs);
+  });
+});

spec/frontend_integration/test_helpers/fixtures.js

@@ -31,6 +31,12 @@ export const getBranch = factory.json(() =>
 export const getMergeRequests = factory.json(() =>
   require('test_fixtures/api/merge_requests/get.json'),
 );
+export const getMergeRequestWithChanges = factory.json(() =>
+  require('test_fixtures/api/merge_requests/changes.json'),
+);
+export const getMergeRequestVersions = factory.json(() =>
+  require('test_fixtures/api/merge_requests/versions.json'),
+);
 export const getRepositoryFiles = factory.json(() =>
   require('test_fixtures/projects_json/files.json'),
 );

spec/frontend_integration/test_helpers/mock_server/index.js

@@ -4,6 +4,8 @@ import {
   getEmptyProject,
   getBranch,
   getMergeRequests,
+  getMergeRequestWithChanges,
+  getMergeRequestVersions,
   getRepositoryFiles,
   getBlobReadme,
   getBlobImage,
@@ -16,6 +18,8 @@ export const createMockServerOptions = () => ({
     project: Model,
     branch: Model,
     mergeRequest: Model,
+    mergeRequestChange: Model,
+    mergeRequestVersion: Model,
     file: Model,
     userPermission: Model,
   },
@@ -30,6 +34,8 @@ export const createMockServerOptions = () => ({
       projects: [getProject(), getEmptyProject()],
       branches: [getBranch()],
       mergeRequests: getMergeRequests(),
+      mergeRequestChanges: [getMergeRequestWithChanges()],
+      mergeRequestVersions: getMergeRequestVersions(),
       filesRaw: [
         {
           raw: getBlobReadme(),

spec/frontend_integration/test_helpers/mock_server/routes/404.js

+import { Response } from 'miragejs';
+
 export default (server) => {
   ['get', 'post', 'put', 'delete', 'patch'].forEach((method) => {
     server[method]('*', () => {

spec/frontend_integration/test_helpers/mock_server/routes/projects.js

@@ -20,4 +20,22 @@ export default (server) => {
 
     return result.models;
   });
+
+  server.get('/api/v4/projects/:id/merge_requests/:mid', (schema, request) => {
+    const mr = schema.mergeRequests.findBy({ iid: request.params.mid });
+
+    return mr.attrs;
+  });
+
+  server.get('/api/v4/projects/:id/merge_requests/:mid/versions', (schema, request) => {
+    const versions = schema.mergeRequestVersions.where({ merge_request_id: request.params.mid });
+
+    return versions.models;
+  });
+
+  server.get('/api/v4/projects/:id/merge_requests/:mid/changes', (schema, request) => {
+    const mrWithChanges = schema.mergeRequestChanges.findBy({ iid: request.params.mid });
+
+    return mrWithChanges.attrs;
+  });
 };