Merge branch 'slashmanov/remove-virtual-scroll-diffs-flag' into 'master'

Remove Virtual Scroll flag from Merge Request Diffs

See merge request gitlab-org/gitlab!79750

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

@@ -53,6 +53,7 @@ import DiffFile from './diff_file.vue';
 import HiddenFilesWarning from './hidden_files_warning.vue';
 import NoChanges from './no_changes.vue';
 import TreeList from './tree_list.vue';
+import VirtualScrollerScrollSync from './virtual_scroller_scroll_sync';
 
 export default {
   name: 'DiffsApp',
@@ -62,8 +63,7 @@ export default {
     DynamicScrollerItem: () =>
       import('vendor/vue-virtual-scroller').then(({ DynamicScrollerItem }) => DynamicScrollerItem),
     PreRenderer: () => import('./pre_renderer.vue').then((PreRenderer) => PreRenderer),
-    VirtualScrollerScrollSync: () =>
-      import('./virtual_scroller_scroll_sync').then((VSSSync) => VSSSync),
+    VirtualScrollerScrollSync,
     CompareVersions,
     DiffFile,
     NoChanges,
@@ -406,10 +406,8 @@ export default {
     this.unsubscribeFromEvents();
     this.removeEventListeners();
 
-    if (window.gon?.features?.diffsVirtualScrolling) {
-      diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash);
-      diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex);
-    }
+    diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash);
+    diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex);
   },
   methods: {
     ...mapActions(['startTaskList']),
@@ -522,32 +520,27 @@ export default {
         );
       }
 
-      if (
-        window.gon?.features?.diffsVirtualScrolling ||
-        window.gon?.features?.usageDataDiffSearches
-      ) {
-        let keydownTime;
-        Mousetrap.bind(['mod+f', 'mod+g'], () => {
-          keydownTime = new Date().getTime();
-        });
+      let keydownTime;
+      Mousetrap.bind(['mod+f', 'mod+g'], () => {
+        keydownTime = new Date().getTime();
+      });
 
-        window.addEventListener('blur', () => {
-          if (keydownTime) {
-            const delta = new Date().getTime() - keydownTime;
+      window.addEventListener('blur', () => {
+        if (keydownTime) {
+          const delta = new Date().getTime() - keydownTime;
 
-            // To make sure the user is using the find function we need to wait for blur
-            // and max 1000ms to be sure it the search box is filtered
-            if (delta >= 0 && delta < 1000) {
-              this.disableVirtualScroller();
+          // To make sure the user is using the find function we need to wait for blur
+          // and max 1000ms to be sure it the search box is filtered
+          if (delta >= 0 && delta < 1000) {
+            this.disableVirtualScroller();
 
-              if (window.gon?.features?.usageDataDiffSearches) {
-                api.trackRedisHllUserEvent('i_code_review_user_searches_diff');
-                api.trackRedisCounterEvent('diff_searches');
-              }
+            if (window.gon?.features?.usageDataDiffSearches) {
+              api.trackRedisHllUserEvent('i_code_review_user_searches_diff');
+              api.trackRedisCounterEvent('diff_searches');
             }
           }
-        });
-      }
+        }
+      });
     },
     removeEventListeners() {
       Mousetrap.unbind(keysFor(MR_PREVIOUS_FILE_IN_DIFF));
@@ -589,8 +582,6 @@ export default {
       this.virtualScrollCurrentIndex = -1;
     },
     scrollVirtualScrollerToDiffNote() {
-      if (!window.gon?.features?.diffsVirtualScrolling) return;
-
       const id = window?.location?.hash;
 
       if (id.startsWith('#note_')) {
@@ -605,11 +596,7 @@ export default {
       }
     },
     subscribeToVirtualScrollingEvents() {
-      if (
-        window.gon?.features?.diffsVirtualScrolling &&
-        this.shouldShow &&
-        !this.subscribedToVirtualScrollingEvents
-      ) {
+      if (this.shouldShow && !this.subscribedToVirtualScrollingEvents) {
         diffsEventHub.$on('scrollToFileHash', this.scrollVirtualScrollerToFileHash);
         diffsEventHub.$on('scrollToIndex', this.scrollVirtualScrollerToIndex);
 

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

@@ -209,7 +209,7 @@ export default {
       handler(val) {
         const el = this.$el.closest('.vue-recycle-scroller__item-view');
 
-        if (this.glFeatures.diffsVirtualScrolling && el) {
+        if (el) {
           // We can't add a style with Vue because of the way the virtual
           // scroller library renders the diff files
           el.style.zIndex = val ? '1' : null;

app/assets/javascripts/diffs/constants.js

@@ -29,8 +29,6 @@ export const UNFOLD_COUNT = 20;
 export const COUNT_OF_AVATARS_IN_GUTTER = 3;
 export const LENGTH_OF_AVATAR_TOOLTIP = 17;
 
-export const LINES_TO_BE_RENDERED_DIRECTLY = 100;
-
 export const DIFF_FILE_SYMLINK_MODE = '120000';
 export const DIFF_FILE_DELETED_MODE = '0';
 

app/assets/javascripts/diffs/index.js

 import Vue from 'vue';
 import { mapActions, mapState, mapGetters } from 'vuex';
-import { getCookie, setCookie, parseBoolean, removeCookie } from '~/lib/utils/common_utils';
+import { getCookie, parseBoolean, removeCookie } from '~/lib/utils/common_utils';
 
-import { getParameterValues } from '~/lib/utils/url_utility';
 import eventHub from '../notes/event_hub';
 import diffsApp from './components/app.vue';
 
@@ -74,11 +73,6 @@ export default function initDiffsApp(store) {
           trackClick: false,
         });
       }
-
-      const vScrollingParam = getParameterValues('virtual_scrolling')[0];
-      if (vScrollingParam === 'false' || vScrollingParam === 'true') {
-        setCookie('diffs_virtual_scrolling', vScrollingParam);
-      }
     },
     methods: {
       ...mapActions('diffs', ['setRenderTreeList', 'setShowWhitespace']),

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

@@ -125,7 +125,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
         commit(types.SET_DIFF_DATA_BATCH, { diff_files });
         commit(types.SET_BATCH_LOADING_STATE, 'loaded');
 
-        if (window.gon?.features?.diffsVirtualScrolling && !scrolledVirtualScroller) {
+        if (!scrolledVirtualScroller) {
           const index = state.diffFiles.findIndex(
             (f) =>
               f.file_hash === hash || f[INLINE_DIFF_LINES_KEY].find((l) => l.line_code === hash),
@@ -195,9 +195,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
         commit(types.SET_BATCH_LOADING_STATE, 'error');
       });
 
-  return getBatch().then(
-    () => !window.gon?.features?.diffsVirtualScrolling && handleLocationHash(),
-  );
+  return getBatch();
 };
 
 export const fetchDiffFilesMeta = ({ commit, state }) => {
@@ -529,7 +527,7 @@ export const setCurrentFileHash = ({ commit }, hash) => {
   commit(types.SET_CURRENT_DIFF_FILE, hash);
 };
 
-export const scrollToFile = ({ state, commit, getters }, { path, setHash = true }) => {
+export const scrollToFile = ({ state, commit, getters }, { path }) => {
   if (!state.treeEntries[path]) return;
 
   const { fileHash } = state.treeEntries[path];
@@ -539,11 +537,9 @@ export const scrollToFile = ({ state, commit, getters }, { path, setHash = true
   if (getters.isVirtualScrollingEnabled) {
     eventHub.$emit('scrollToFileHash', fileHash);
 
-    if (setHash) {
-      setTimeout(() => {
-        window.history.replaceState(null, null, `#${fileHash}`);
-      });
-    }
+    setTimeout(() => {
+      window.history.replaceState(null, null, `#${fileHash}`);
+    });
   } else {
     document.location.hash = fileHash;
 

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

-import { getCookie } from '~/lib/utils/common_utils';
-import { getParameterValues } from '~/lib/utils/url_utility';
 import { __, n__ } from '~/locale';
+import { getParameterValues } from '~/lib/utils/url_utility';
 import {
   PARALLEL_DIFF_VIEW_TYPE,
   INLINE_DIFF_VIEW_TYPE,
@@ -175,21 +174,11 @@ export function suggestionCommitMessage(state, _, rootState) {
 }
 
 export const isVirtualScrollingEnabled = (state) => {
-  const vSrollerCookie = getCookie('diffs_virtual_scrolling');
-
-  if (state.disableVirtualScroller) {
+  if (state.disableVirtualScroller || getParameterValues('virtual_scrolling')[0] === 'false') {
     return false;
   }
 
-  if (vSrollerCookie) {
-    return vSrollerCookie === 'true';
-  }
-
-  return (
-    !state.viewDiffsFileByFile &&
-    (window.gon?.features?.diffsVirtualScrolling ||
-      getParameterValues('virtual_scrolling')[0] === 'true')
-  );
+  return !state.viewDiffsFileByFile;
 };
 
 export const isBatchLoading = (state) => state.batchLoadingState === 'loading';

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

@@ -9,7 +9,6 @@ import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
   MATCH_LINE_TYPE,
-  LINES_TO_BE_RENDERED_DIRECTLY,
   INLINE_DIFF_LINES_KEY,
   CONFLICT_OUR,
   CONFLICT_THEIR,
@@ -380,16 +379,9 @@ function prepareDiffFileLines(file) {
   return file;
 }
 
-function finalizeDiffFile(file, index) {
-  let renderIt = Boolean(window.gon?.features?.diffsVirtualScrolling);
-
-  if (!window.gon?.features?.diffsVirtualScrolling) {
-    renderIt =
-      index < 3 ? file[INLINE_DIFF_LINES_KEY].length < LINES_TO_BE_RENDERED_DIRECTLY : false;
-  }
-
+function finalizeDiffFile(file) {
   Object.assign(file, {
-    renderIt,
+    renderIt: true,
     isShowingFullFile: false,
     isLoadingFullFile: false,
     discussions: [],
@@ -417,15 +409,13 @@ export function prepareDiffData({ diff, priorFiles = [], meta = false }) {
     .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles, meta }))
     .map(ensureBasicDiffFileLines)
     .map(prepareDiffFileLines)
-    .map((file, index) => finalizeDiffFile(file, priorFiles.length + index));
+    .map((file) => finalizeDiffFile(file));
 
   return deduplicateFilesList([...priorFiles, ...cleanedFiles]);
 }
 
 export function getDiffPositionByLineCode(diffFiles) {
-  let lines = [];
-
-  lines = diffFiles.reduce((acc, diffFile) => {
+  const lines = diffFiles.reduce((acc, diffFile) => {
     diffFile[INLINE_DIFF_LINES_KEY].forEach((line) => {
       acc.push({ file: diffFile, line });
     });

app/assets/javascripts/notes/mixins/discussion_navigation.js

@@ -3,8 +3,6 @@ import { scrollToElementWithContext, scrollToElement } from '~/lib/utils/common_
 import { updateHistory } from '../../lib/utils/url_utility';
 import eventHub from '../event_hub';
 
-const isDiffsVirtualScrollingEnabled = () => window.gon?.features?.diffsVirtualScrolling;
-
 /**
  * @param {string} selector
  * @returns {boolean}
@@ -15,7 +13,7 @@ function scrollTo(selector, { withoutContext = false } = {}) {
 
   if (el) {
     scrollFunction(el, {
-      behavior: isDiffsVirtualScrollingEnabled() ? 'auto' : 'smooth',
+      behavior: 'auto',
     });
     return true;
   }
@@ -31,7 +29,7 @@ function updateUrlWithNoteId(noteId) {
     replace: true,
   };
 
-  if (noteId && isDiffsVirtualScrollingEnabled()) {
+  if (noteId) {
     // Temporarily mask the ID to avoid the browser default
     //    scrolling taking over which is broken with virtual
     //    scrolling enabled.
@@ -115,17 +113,13 @@ function handleDiscussionJump(self, fn, discussionId = self.currentDiscussionId)
   const isDiffView = window.mrTabs.currentAction === 'diffs';
   const targetId = fn(discussionId, isDiffView);
   const discussion = self.getDiscussion(targetId);
-  const setHash = !isDiffView && !isDiffsVirtualScrollingEnabled();
   const discussionFilePath = discussion?.diff_file?.file_path;
 
-  if (isDiffsVirtualScrollingEnabled()) {
-    window.location.hash = '';
-  }
+  window.location.hash = '';
 
   if (discussionFilePath) {
     self.scrollToFile({
       path: discussionFilePath,
-      setHash,
     });
   }
 

app/controllers/projects/merge_requests_controller.rb

@@ -42,7 +42,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:paginated_notes, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:confidential_notes, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:improved_emoji_picker, project, default_enabled: :yaml)
-    push_frontend_feature_flag(:diffs_virtual_scrolling, project, default_enabled: :yaml)
     push_frontend_feature_flag(:restructured_mr_widget, project, default_enabled: :yaml)
     push_frontend_feature_flag(:refactor_mr_widgets_extensions, @project, default_enabled: :yaml)
     push_frontend_feature_flag(:rebase_without_ci_ui, @project, default_enabled: :yaml)

config/feature_flags/development/diffs_virtual_scrolling.yml

----
-name: diffs_virtual_scrolling
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60312
-rollout_issue_url:
-milestone: '13.12'
-type: development
-group: group::code review
-default_enabled: true

spec/features/merge_request/batch_comments_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe 'Merge request > Batch comments', :js do
 
       expect(page).to have_selector('[data-testid="review_bar_component"]')
 
-      expect(find('.review-bar-content .btn-confirm')).to have_content('1')
+      expect(find('[data-testid="review_bar_component"] .btn-confirm')).to have_content('1')
     end
 
     it 'publishes review' do
@@ -150,10 +150,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
 
       it 'adds draft comments to both sides' do
         write_parallel_comment('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9')
-
-        # make sure line 9 is in the view
-        execute_script("window.scrollBy(0, -200)")
-
         write_parallel_comment('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9', button_text: 'Add to review', text: 'Another wrong line')
 
         expect(find('.new .draft-note-component')).to have_content('Line is wrong')
@@ -255,13 +251,15 @@ RSpec.describe 'Merge request > Batch comments', :js do
   end
 
   def write_diff_comment(**params)
-    click_diff_line(find("[id='#{sample_compare.changes[0][:line_code]}']"))
+    click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']"))
 
     write_comment(**params)
   end
 
   def write_parallel_comment(line, **params)
-    find("div[id='#{line}']").hover
+    line_element = find_by_scrolling("[id='#{line}']")
+    scroll_to_elements_bottom(line_element)
+    line_element.hover
     find(".js-add-diff-note-button").click
 
     write_comment(selector: "form[data-line-code='#{line}']", **params)

spec/features/merge_request/user_comments_on_diff_spec.rb

@@ -25,14 +25,15 @@ RSpec.describe 'User comments on a diff', :js do
     context 'when toggling inline comments' do
       context 'in a single file' do
         it 'hides a comment' do
-          click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+          line_element = find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']").find(:xpath, "..")
+          click_diff_line(line_element)
 
           page.within('.js-discussion-note-form') do
             fill_in('note_note', with: 'Line is wrong')
             click_button('Add comment now')
           end
 
-          page.within('.diff-files-holder > div:nth-child(6)') do
+          page.within(line_element.ancestor('[data-path]')) do
             expect(page).to have_content('Line is wrong')
 
             find('.js-diff-more-actions').click
@@ -45,7 +46,9 @@ RSpec.describe 'User comments on a diff', :js do
 
       context 'in multiple files' do
         it 'toggles comments' do
-          click_diff_line(find("[id='#{sample_compare.changes[0][:line_code]}']"))
+          first_line_element = find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']").find(:xpath, "..")
+          first_root_element = first_line_element.ancestor('[data-path]')
+          click_diff_line(first_line_element)
 
           page.within('.js-discussion-note-form') do
             fill_in('note_note', with: 'Line is correct')
@@ -54,11 +57,14 @@ RSpec.describe 'User comments on a diff', :js do
 
           wait_for_requests
 
-          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
+          page.within(first_root_element) do
             expect(page).to have_content('Line is correct')
           end
 
-          click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+          second_line_element = find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")
+          second_root_element = second_line_element.ancestor('[data-path]')
+
+          click_diff_line(second_line_element)
 
           page.within('.js-discussion-note-form') do
             fill_in('note_note', with: 'Line is wrong')
@@ -68,7 +74,7 @@ RSpec.describe 'User comments on a diff', :js do
           wait_for_requests
 
           # Hide the comment.
-          page.within('.diff-files-holder > div:nth-child(6)') do
+          page.within(second_root_element) do
             find('.js-diff-more-actions').click
             click_button 'Hide comments on this file'
 
@@ -77,37 +83,36 @@ RSpec.describe 'User comments on a diff', :js do
 
           # At this moment a user should see only one comment.
           # The other one should be hidden.
-          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
+          page.within(first_root_element) do
             expect(page).to have_content('Line is correct')
           end
 
           # Show the comment.
-          page.within('.diff-files-holder > div:nth-child(6)') do
+          page.within(second_root_element) do
             find('.js-diff-more-actions').click
             click_button 'Show comments on this file'
           end
 
           # Now both the comments should be shown.
-          page.within('.diff-files-holder > div:nth-child(6) .note-body > .note-text') do
+          page.within(second_root_element) do
             expect(page).to have_content('Line is wrong')
           end
 
-          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
+          page.within(first_root_element) do
             expect(page).to have_content('Line is correct')
           end
 
           # Check the same comments in the side-by-side view.
-          execute_script("window.scrollTo(0,0);")
           find('.js-show-diff-settings').click
           click_button 'Side-by-side'
 
           wait_for_requests
 
-          page.within('.diff-files-holder > div:nth-child(6) .parallel .note-body > .note-text') do
+          page.within(second_root_element) do
             expect(page).to have_content('Line is wrong')
           end
 
-          page.within('.diff-files-holder > div:nth-child(5) .parallel .note-body > .note-text') do
+          page.within(first_root_element) do
             expect(page).to have_content('Line is correct')
           end
         end
@@ -121,7 +126,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'when adding multiline comments' do
     it 'saves a multiline comment' do
-      click_diff_line(find("[id='#{sample_commit.line_code}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']").find(:xpath, '..'))
       add_comment('-13', '+14')
     end
 
@@ -133,13 +138,13 @@ RSpec.describe 'User comments on a diff', :js do
       # In `files/ruby/popen.rb`
       it 'allows comments for changes involving both sides' do
         # click +15, select -13 add and verify comment
-        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .right-side a[data-linenumber="15"]').find(:xpath, '../../..'), 'right')
+        click_diff_line(find_by_scrolling('div[data-path="files/ruby/popen.rb"] .right-side a[data-linenumber="15"]').find(:xpath, '../../..'), 'right')
         add_comment('-13', '+15')
       end
 
       it 'allows comments on previously hidden lines at the top of a file', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/285294' do
         # Click -9, expand up, select 1 add and verify comment
-        page.within('[data-path="files/ruby/popen.rb"]') do
+        page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do
           all('.js-unfold-all')[0].click
         end
         click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
@@ -148,7 +153,7 @@ RSpec.describe 'User comments on a diff', :js do
 
       it 'allows comments on previously hidden lines the middle of a file' do
         # Click 27, expand up, select 18, add and verify comment
-        page.within('[data-path="files/ruby/popen.rb"]') do
+        page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do
           all('.js-unfold-all')[1].click
         end
         click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
@@ -157,7 +162,7 @@ RSpec.describe 'User comments on a diff', :js do
 
       it 'allows comments on previously hidden lines at the bottom of a file' do
         # Click +28, expand down, select 37 add and verify comment
-        page.within('[data-path="files/ruby/popen.rb"]') do
+        page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do
           all('.js-unfold-down:not([disabled])')[1].click
         end
         click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
@@ -198,7 +203,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'when editing comments' do
     it 'edits a comment' do
-      click_diff_line(find("[id='#{sample_commit.line_code}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in(:note_note, with: 'Line is wrong')
@@ -224,7 +229,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'when deleting comments' do
     it 'deletes a comment' do
-      click_diff_line(find("[id='#{sample_commit.line_code}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in(:note_note, with: 'Line is wrong')

spec/features/merge_request/user_expands_diff_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe 'User expands diff', :js do
   let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) }
 
   before do
-    allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(100.kilobytes)
+    allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(100.bytes)
 
     visit(diffs_project_merge_request_path(project, merge_request))
 
@@ -15,7 +15,7 @@ RSpec.describe 'User expands diff', :js do
   end
 
   it 'allows user to expand diff' do
-    page.within find('[id="19763941ab80e8c09871c0a425f0560d9053bcb3"]') do
+    page.within find("[id='4c76a1271e41072d7da9fe40bf0f79f7384d472a']") do
       find('[data-testid="expand-button"]').click
 
       wait_for_requests

spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb

@@ -15,16 +15,14 @@ RSpec.describe 'Batch diffs', :js do
     visit diffs_project_merge_request_path(merge_request.project, merge_request)
     wait_for_requests
 
-    # Add discussion to first line of first file
-    click_diff_line(find('.diff-file.file-holder:first-of-type .line_holder .left-side:first-of-type'))
-    page.within('.js-discussion-note-form') do
+    click_diff_line(get_first_diff.find('[data-testid="left-side"]', match: :first))
+    page.within get_first_diff.find('.js-discussion-note-form') do
       fill_in('note_note', with: 'First Line Comment')
       click_button('Add comment now')
     end
 
-    # Add discussion to first line of last file
-    click_diff_line(find('.diff-file.file-holder:last-of-type .line_holder .left-side:first-of-type'))
-    page.within('.js-discussion-note-form') do
+    click_diff_line(get_second_diff.find('[data-testid="left-side"]', match: :first))
+    page.within get_second_diff.find('.js-discussion-note-form') do
       fill_in('note_note', with: 'Last Line Comment')
       click_button('Add comment now')
     end
@@ -36,17 +34,14 @@ RSpec.describe 'Batch diffs', :js do
     # Reload so we know the discussions are persisting across batch loads
     visit page.current_url
 
-    # Wait for JS to settle
     wait_for_requests
 
-    expect(page).to have_selector('.diff-files-holder .file-holder', count: 39)
-
     # Confirm discussions are applied to appropriate files (should be contained in multiple diff pages)
-    page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do
+    page.within get_first_diff.find('.notes .timeline-entry .note .note-text') do
       expect(page).to have_content('First Line Comment')
     end
 
-    page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do
+    page.within get_second_diff.find('.notes .timeline-entry .note .note-text') do
       expect(page).to have_content('Last Line Comment')
     end
   end
@@ -54,7 +49,7 @@ RSpec.describe 'Batch diffs', :js do
   context 'when user visits a URL with a link directly to to a discussion' do
     context 'which is in the first batched page of diffs' do
       it 'scrolls to the correct discussion' do
-        page.within('.diff-file.file-holder:first-of-type') do
+        page.within get_first_diff do
           click_link('just now')
         end
 
@@ -63,15 +58,15 @@ RSpec.describe 'Batch diffs', :js do
         wait_for_requests
 
         # Confirm scrolled to correct UI element
-        expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey
-        expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy
+        expect(get_first_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey
+        expect(get_second_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy
       end
     end
 
     context 'which is in at least page 2 of the batched pages of diffs' do
       it 'scrolls to the correct discussion',
          quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/293814' } do
-        page.within('.diff-file.file-holder:last-of-type') do
+        page.within get_first_diff do
           click_link('just now')
         end
 
@@ -80,8 +75,8 @@ RSpec.describe 'Batch diffs', :js do
         wait_for_requests
 
         # Confirm scrolled to correct UI element
-        expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy
-        expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey
+        expect(get_first_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy
+        expect(get_second_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey
       end
     end
   end
@@ -95,15 +90,21 @@ RSpec.describe 'Batch diffs', :js do
     end
 
     it 'has the correct discussions applied to files across batched pages' do
-      expect(page).to have_selector('.diff-files-holder .file-holder', count: 39)
-
-      page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do
+      page.within get_first_diff.find('.notes .timeline-entry .note .note-text') do
         expect(page).to have_content('First Line Comment')
       end
 
-      page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do
+      page.within get_second_diff.find('.notes .timeline-entry .note .note-text') do
         expect(page).to have_content('Last Line Comment')
       end
     end
   end
+
+  def get_first_diff
+    find('#a9b6f940524f646951cc28d954aa41f814f95d4f')
+  end
+
+  def get_second_diff
+    find('#b285a86891571c7fdbf1f82e840816079de1cc8b')
+  end
 end

spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb

@@ -6,6 +6,7 @@ include Spec::Support::Helpers::ModalHelpers # rubocop:disable  Style/MixinUsage
 RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
   include NoteInteractionHelpers
   include Spec::Support::Helpers::ModalHelpers
+  include MergeRequestDiffHelpers
 
   let(:project)       { create(:project, :public, :repository) }
   let(:user)          { project.creator }
@@ -135,6 +136,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
       end
 
       it 'adds avatar when commenting' do
+        find_by_scrolling('[data-discussion-id]', match: :first)
         find_field('Reply…', match: :first).click
 
         page.within '.js-discussion-note-form' do
@@ -154,6 +156,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
 
       it 'adds multiple comments' do
         3.times do
+          find_by_scrolling('[data-discussion-id]', match: :first)
           find_field('Reply…', match: :first).click
 
           page.within '.js-discussion-note-form' do
@@ -192,7 +195,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do
   end
 
   def find_line(line_code)
-    line = find("[id='#{line_code}']")
+    line = find_by_scrolling("[id='#{line_code}']")
     line = line.find(:xpath, 'preceding-sibling::*[1][self::td]/preceding-sibling::*[1][self::td]') if line.tag_name == 'td'
     line
   end

spec/features/merge_request/user_sees_diff_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe 'Merge request > User sees diff', :js do
   include ProjectForksHelper
   include RepoHelpers
+  include MergeRequestDiffHelpers
 
   let(:project) { create(:project, :public, :repository) }
   let(:merge_request) { create(:merge_request, source_project: project) }
@@ -58,12 +59,12 @@ RSpec.describe 'Merge request > User sees diff', :js do
     let(:changelog_id) { Digest::SHA1.hexdigest("CHANGELOG") }
 
     context 'as author' do
-      it 'shows direct edit link', :sidekiq_might_not_need_inline do
+      it 'contains direct edit link', :sidekiq_might_not_need_inline do
         sign_in(author_user)
         visit diffs_project_merge_request_path(project, merge_request)
 
         # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax
-        expect(page).to have_selector("[id=\"#{changelog_id}\"] .js-edit-blob", visible: false)
+        expect(page).to have_selector(".js-edit-blob", visible: false)
       end
     end
 
@@ -72,6 +73,8 @@ RSpec.describe 'Merge request > User sees diff', :js do
         sign_in(user)
         visit diffs_project_merge_request_path(project, merge_request)
 
+        find_by_scrolling("[id=\"#{changelog_id}\"]")
+
         # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax
         find("[id=\"#{changelog_id}\"] .js-diff-more-actions").click
         find("[id=\"#{changelog_id}\"] .js-edit-blob").click
@@ -107,6 +110,7 @@ RSpec.describe 'Merge request > User sees diff', :js do
           CONTENT
 
         file_name = 'xss_file.rs'
+        file_hash = Digest::SHA1.hexdigest(file_name)
 
         create_file('master', file_name, file_content)
         merge_request = create(:merge_request, source_project: project)
@@ -116,6 +120,8 @@ RSpec.describe 'Merge request > User sees diff', :js do
 
         visit diffs_project_merge_request_path(project, merge_request)
 
+        find_by_scrolling("[id='#{file_hash}']")
+
         expect(page).to have_text("function foo<input> {")
         expect(page).to have_css(".line[lang='rust'] .k")
       end
@@ -136,7 +142,7 @@ RSpec.describe 'Merge request > User sees diff', :js do
         end
 
         context 'when file is an image', :js do
-          let(:file_name) { 'files/lfs/image.png' }
+          let(:file_name) { 'a/image.png' }
 
           it 'shows an error message' do
             expect(page).not_to have_content('could not be displayed because it is stored in LFS')
@@ -144,7 +150,7 @@ RSpec.describe 'Merge request > User sees diff', :js do
         end
 
         context 'when file is not an image' do
-          let(:file_name) { 'files/lfs/ruby.rb' }
+          let(:file_name) { 'a/ruby.rb' }
 
           it 'shows an error message' do
             expect(page).to have_content('This source diff could not be displayed because it is stored in LFS')
@@ -153,7 +159,14 @@ RSpec.describe 'Merge request > User sees diff', :js do
       end
 
       context 'when LFS is not enabled' do
+        let(:file_name) { 'a/lfs_object.iso' }
+
         before do
+          allow(Gitlab.config.lfs).to receive(:disabled).and_return(true)
+          project.update_attribute(:lfs_enabled, false)
+
+          create_file('master', file_name, project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data)
+
           visit diffs_project_merge_request_path(project, merge_request)
         end
 

spec/features/merge_request/user_sees_versions_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe 'Merge request > User sees versions', :js do
+  include MergeRequestDiffHelpers
+
   let(:merge_request) do
     create(:merge_request).tap do |mr|
       mr.merge_request_diff.destroy!
@@ -27,8 +29,12 @@ RSpec.describe 'Merge request > User sees versions', :js do
       diff_file_selector = ".diff-file[id='#{file_id}']"
       line_code = "#{file_id}_#{line_code}"
 
-      page.within(diff_file_selector) do
-        first("[id='#{line_code}']").hover
+      page.within find_by_scrolling(diff_file_selector) do
+        line_code_element = first("[id='#{line_code}']")
+        # scrolling to element's bottom is required in order for .hover action to work
+        # otherwise, the element could be hidden underneath a sticky header
+        scroll_to_elements_bottom(line_code_element)
+        line_code_element.hover
         first("[id='#{line_code}'] [role='button']").click
 
         page.within("form[data-line-code='#{line_code}']") do

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'single suggestion note' do
     it 'hides suggestion popover' do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       expect(page).to have_selector('.diff-suggest-popover')
 
@@ -46,7 +46,7 @@ RSpec.describe 'User comments on a diff', :js do
     end
 
     it 'suggestion is presented' do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
@@ -74,7 +74,7 @@ RSpec.describe 'User comments on a diff', :js do
     end
 
     it 'allows suggestions in replies' do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
@@ -91,7 +91,7 @@ RSpec.describe 'User comments on a diff', :js do
     end
 
     it 'suggestion is appliable' do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
@@ -273,7 +273,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'multiple suggestions in a single note' do
     it 'suggestions are presented', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/258989' do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion:-2\n# or that\n# heh\n```")
@@ -316,7 +316,7 @@ RSpec.describe 'User comments on a diff', :js do
 
   context 'multi-line suggestions' do
     before do
-      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+      click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']"))
 
       page.within('.js-discussion-note-form') do
         fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```")

spec/frontend/diffs/components/app_spec.js

@@ -69,6 +69,12 @@ describe('diffs/components/app', () => {
       },
       provide,
       store,
+      stubs: {
+        DynamicScroller: {
+          template: `<div><slot :item="$store.state.diffs.diffFiles[0]"></slot></div>`,
+        },
+        DynamicScrollerItem: true,
+      },
     });
   }
 

spec/frontend/diffs/store/actions_spec.js

@@ -202,7 +202,7 @@ describe('DiffsStoreActions', () => {
       testAction(
         fetchDiffFilesBatch,
         {},
-        { endpointBatch, diffViewType: 'inline' },
+        { endpointBatch, diffViewType: 'inline', diffFiles: [] },
         [
           { type: types.SET_BATCH_LOADING_STATE, payload: 'loading' },
           { type: types.SET_RETRIEVING_BATCHES, payload: true },

spec/frontend/notes/mixins/discussion_navigation_spec.js

@@ -143,7 +143,7 @@ describe('Discussion navigation mixin', () => {
         it('scrolls to element', () => {
           expect(utils.scrollToElement).toHaveBeenCalledWith(
             findDiscussion('div.discussion', expected),
-            { behavior: 'smooth' },
+            { behavior: 'auto' },
           );
         });
       });
@@ -171,7 +171,7 @@ describe('Discussion navigation mixin', () => {
 
           expect(utils.scrollToElementWithContext).toHaveBeenCalledWith(
             findDiscussion('ul.notes', expected),
-            { behavior: 'smooth' },
+            { behavior: 'auto' },
           );
         });
       });
@@ -212,21 +212,15 @@ describe('Discussion navigation mixin', () => {
           it('scrolls to discussion', () => {
             expect(utils.scrollToElement).toHaveBeenCalledWith(
               findDiscussion('div.discussion', expected),
-              { behavior: 'smooth' },
+              { behavior: 'auto' },
             );
           });
         });
       });
     });
 
-    describe.each`
-      diffsVirtualScrolling
-      ${false}
-      ${true}
-    `('virtual scrolling feature is $diffsVirtualScrolling', ({ diffsVirtualScrolling }) => {
+    describe('virtual scrolling feature', () => {
       beforeEach(() => {
-        window.gon = { features: { diffsVirtualScrolling } };
-
         jest.spyOn(store, 'dispatch');
 
         store.state.notes.currentDiscussionId = 'a';
@@ -238,22 +232,22 @@ describe('Discussion navigation mixin', () => {
         window.location.hash = '';
       });
 
-      it('resets location hash if diffsVirtualScrolling flag is true', async () => {
+      it('resets location hash', async () => {
         wrapper.vm.jumpToNextDiscussion();
 
         await nextTick();
 
-        expect(window.location.hash).toBe(diffsVirtualScrolling ? '' : '#test');
+        expect(window.location.hash).toBe('');
       });
 
       it.each`
-        tabValue   | hashValue
-        ${'diffs'} | ${false}
-        ${'show'}  | ${!diffsVirtualScrolling}
-        ${'other'} | ${!diffsVirtualScrolling}
+        tabValue
+        ${'diffs'}
+        ${'show'}
+        ${'other'}
       `(
         'calls scrollToFile with setHash as $hashValue when the tab is $tabValue',
-        async ({ hashValue, tabValue }) => {
+        async ({ tabValue }) => {
           window.mrTabs.currentAction = tabValue;
 
           wrapper.vm.jumpToNextDiscussion();
@@ -262,7 +256,6 @@ describe('Discussion navigation mixin', () => {
 
           expect(store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', {
             path: 'test.js',
-            setHash: hashValue,
           });
         },
       );

spec/spec_helper.rb

@@ -294,8 +294,6 @@ RSpec.configure do |config|
       # See https://gitlab.com/gitlab-org/gitlab/-/issues/33867
       stub_feature_flags(file_identifier_hash: false)
 
-      stub_feature_flags(diffs_virtual_scrolling: false)
-
       # The following `vue_issues_list` stub can be removed
       # once the Vue issues page has feature parity with the current Haml page
       stub_feature_flags(vue_issues_list: false)

spec/support/helpers/merge_request_diff_helpers.rb

 # frozen_string_literal: true
 
 module MergeRequestDiffHelpers
+  PageEndReached = Class.new(StandardError)
+
   def click_diff_line(line_holder, diff_side = nil)
     line = get_line_components(line_holder, diff_side)
+    scroll_to_elements_bottom(line_holder)
     line_holder.hover
     line[:num].find('.js-add-diff-note-button').click
   end
@@ -27,4 +30,55 @@ module MergeRequestDiffHelpers
     line_holder.find('.diff-line-num', match: :first)
     { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] }
   end
+
+  def has_reached_page_end
+    evaluate_script("(window.innerHeight + window.scrollY) >= document.body.offsetHeight")
+  end
+
+  def scroll_to_elements_bottom(element)
+    evaluate_script("(function(el) {
+      window.scrollBy(0, el.getBoundingClientRect().bottom - window.innerHeight);
+    })(arguments[0]);", element.native)
+  end
+
+  # we're not using Capybara's .obscured here because it also checks if the element is clickable
+  def within_viewport?(element)
+    evaluate_script("(function(el) {
+      var rect = el.getBoundingClientRect();
+      return (
+        rect.bottom >= 0 &&
+        rect.right >= 0 &&
+        rect.top <= (window.innerHeight || document.documentElement.clientHeight) &&
+        rect.left <= (window.innerWidth || document.documentElement.clientWidth)
+      );
+    })(arguments[0]);", element.native)
+  end
+
+  def find_within_viewport(selector, **options)
+    begin
+      element = find(selector, **options, wait: 2)
+    rescue Capybara::ElementNotFound
+      return
+    end
+    return element if within_viewport?(element)
+
+    nil
+  end
+
+  def find_by_scrolling(selector, **options)
+    element = find_within_viewport(selector, **options)
+    return element if element
+
+    page.execute_script "window.scrollTo(0,0)"
+    until element
+
+      if has_reached_page_end
+        raise PageEndReached, "Failed to find any elements matching a selector '#{selector}' by scrolling. Page end reached."
+      end
+
+      page.execute_script "window.scrollBy(0,window.innerHeight/1.5)"
+      element = find_within_viewport(selector, **options)
+    end
+    element
+  end
 end

spec/support/helpers/note_interaction_helpers.rb

 # frozen_string_literal: true
 
 module NoteInteractionHelpers
+  include MergeRequestDiffHelpers
+
   def open_more_actions_dropdown(note)
-    note_element = find("#note_#{note.id}")
+    note_element = find_by_scrolling("#note_#{note.id}")
 
     note_element.find('.more-actions-toggle').click
     note_element.find('.more-actions .dropdown-menu li', match: :first)

spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb

@@ -2,7 +2,7 @@
 
 RSpec.shared_examples 'comment on merge request file' do
   it 'adds a comment' do
-    click_diff_line(find("[id='#{sample_commit.line_code}']"))
+    click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']"))
 
     page.within('.js-discussion-note-form') do
       fill_in(:note_note, with: 'Line is wrong')