Follow-up from "Fix authoritative source of truth for diff files when loading batched diffs"

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Work on this issue
• Close this issue

The following discussion from !23274 (merged) should be addressed:

• @pslaughter started a discussion:

  suggestion (non-blocking): This business logic is a little confusing... It seems like under certain conditions, the diffFiles will never be updated, but it just so happens that another mutation SET_DIFF_DATA_BATCH (which is very similar to this one) is actually handling the work.

  We could probably benefit from having a SET_DIFF_META_DATA which updates everything but the diffFiles. We could then call this new mutation in fetchDiffFilesMeta and remove this conditional along with deduping SET_DIFF_DATA_BATCH and SET_DIFF_DATA.

  Untested patch

  diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
  index 31708cc4aa7..5964e634b73 100644
  --- a/app/assets/javascripts/diffs/store/actions.js
  +++ b/app/assets/javascripts/diffs/store/actions.js
  @@ -84,7 +84,7 @@ export const fetchDiffFiles = ({ state, commit }) => {
         commit(types.SET_LOADING, false);
   
         commit(types.SET_MERGE_REQUEST_DIFFS, res.data.merge_request_diffs || []);
  -      commit(types.SET_DIFF_DATA, res.data);
  +      commit(types.SET_DIFF_DATA_BATCH, res.data);
   
         worker.postMessage(state.diffFiles);
   
  @@ -163,7 +163,7 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
         delete strippedData.diff_files;
         commit(types.SET_LOADING, false);
         commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []);
  -      commit(types.SET_DIFF_DATA, strippedData);
  +      commit(types.SET_DIFF_META_DATA, strippedData);
   
         worker.postMessage(prepareDiffData({ diff: data, priorFiles: state.diffFiles }));
   
  diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js
  index 2097c8d3655..46d3dd32ef3 100644
  --- a/app/assets/javascripts/diffs/store/mutation_types.js
  +++ b/app/assets/javascripts/diffs/store/mutation_types.js
  @@ -2,7 +2,7 @@ export const SET_BASE_CONFIG = 'SET_BASE_CONFIG';
   export const SET_LOADING = 'SET_LOADING';
   export const SET_BATCH_LOADING = 'SET_BATCH_LOADING';
   export const SET_RETRIEVING_BATCHES = 'SET_RETRIEVING_BATCHES';
  -export const SET_DIFF_DATA = 'SET_DIFF_DATA';
  +export const SET_DIFF_META_DATA = 'SET_DIFF_META_DATA';
   export const SET_DIFF_DATA_BATCH = 'SET_DIFF_DATA_BATCH';
   export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';
   export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS';
  diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
  index 5a9642cca74..fb1fe96abb6 100644
  --- a/app/assets/javascripts/diffs/store/mutations.js
  +++ b/app/assets/javascripts/diffs/store/mutations.js
  @@ -44,19 +44,9 @@ export default {
       Object.assign(state, { retrievingBatches });
     },
   
  -  [types.SET_DIFF_DATA](state, data) {
  -    let files = state.diffFiles;
  -
  -    if (
  -      !(gon?.features?.diffsBatchLoad && window.location.search.indexOf('diff_id') === -1) &&
  -      data.diff_files
  -    ) {
  -      files = prepareDiffData({ diff: data, priorFiles: files });
  -    }
  -
  +  [types.SET_DIFF_META_DATA](state, { diff_files, ...data }) {
       Object.assign(state, {
         ...convertObjectPropsToCamelCase(data),
  -      diffFiles: files,
       });
     },
   
  

  I know this isn't related to this MR. Just leaving some thoughts... 😄

  Can we create a follow up issue for this?