Fix IDE openMergeRequest to route to first file

- This causes the file to be shown active in the
  file tree
- Also removes setFileMrChange action since it
  isnt used now that we directly use the mutation

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

@@ -147,70 +147,94 @@ 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, 10).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/ide/stores/actions/merge_request_spec.js

 import MockAdapter from 'axios-mock-adapter';
+import { range } from 'lodash';
+import testAction from 'helpers/vuex_action_helper';
+import { TEST_HOST } from 'helpers/test_constants';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
 import { leftSidebarViews, PERMISSION_READ_MR } from '~/ide/constants';
 import service from '~/ide/services';
@@ -7,13 +10,24 @@ 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, 10).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);