diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index a8ca17ab4dd590ae41d7b37f8db9654502b5ce72..5707e4d67f937350e41bad83a3cf7d2d1fe1cdd4 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/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);
diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue
index 0e8506cd896baecb628a15e8bd653c4874b5679c..3cf1f69b08c1860ed200b8afd852255dac71ef04 100644
--- a/app/assets/javascripts/diffs/components/diff_file_header.vue
+++ b/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;
diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js
index c2eefad8f404668e6214385e3f046213401c4fa3..bbe27c0dbd6a7dda0d2e61fc0b1262c348356442 100644
--- a/app/assets/javascripts/diffs/constants.js
+++ b/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';
diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js
index dfe29e767c96b2c89dd67d06bfe7140455cb3971..1691da34c6ddf90f77238b16fb8c1742dc376d7f 100644
--- a/app/assets/javascripts/diffs/index.js
+++ b/app/assets/javascripts/diffs/index.js
@@ -1,8 +1,7 @@
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']),
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 572949e585ad90ebea9e5e63aaa5b647962f21f7..e967be23f42af596f8ebe86dd14594a598ac9519 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/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;
diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js
index 83c7f8c814b5801d93d08bbb77026ef4fa365ebc..3a85c1a9fe140538eaa1402f74e011a8dfc34209 100644
--- a/app/assets/javascripts/diffs/store/getters.js
+++ b/app/assets/javascripts/diffs/store/getters.js
@@ -1,6 +1,5 @@
-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';
diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index 3f1af68e37a5500da6b794eb7d2e21b2d284469d..f2028892a5ffeaaf8810cc8eb2c3c44b2d7f1ad2 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/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 });
});
diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js
index ad529eb99b6b8acd6edc14e9950f8e1575296785..93236b05100d23d6392eecf115a6dfabeb7622cd 100644
--- a/app/assets/javascripts/notes/mixins/discussion_navigation.js
+++ b/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,
});
}
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 103622646ab3d6198bc7a43fb390a28eefb1e5e1..05452ef65786bcd9e4fb4677fa814fbd250a202c 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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)
diff --git a/config/feature_flags/development/diffs_virtual_scrolling.yml b/config/feature_flags/development/diffs_virtual_scrolling.yml
deleted file mode 100644
index add1297d8b8e6957e7b49f8b9d0e64e489b97823..0000000000000000000000000000000000000000
--- a/config/feature_flags/development/diffs_virtual_scrolling.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-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
diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb
index a4aae7d8020f2b3e9bf1207f6ffb3094f0060717..eb98c7d5061f1a8454ba6058b3a12f4319fb1c9b 100644
--- a/spec/features/merge_request/batch_comments_spec.rb
+++ b/spec/features/merge_request/batch_comments_spec.rb
@@ -32,7 +32,7 @@
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 @@
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 @@ def visit_overview
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)
diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb
index 29c346593d507b6f0b17eb9e980141e3aad245fa..c06019f6b77429660c5ca79e3b23edf25a87b431 100644
--- a/spec/features/merge_request/user_comments_on_diff_spec.rb
+++ b/spec/features/merge_request/user_comments_on_diff_spec.rb
@@ -25,14 +25,15 @@
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 @@
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 @@
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 @@
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 @@
# 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 @@
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 @@
# 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 @@
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 @@
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 @@ def add_comment(start_line, end_line)
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 @@ def add_comment(start_line, end_line)
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')
diff --git a/spec/features/merge_request/user_expands_diff_spec.rb b/spec/features/merge_request/user_expands_diff_spec.rb
index 52554f11d287bd1af1c613d3ac484bd8dcab378d..25c9584350d3e7c5b79c2640c5b77a85147d5b3d 100644
--- a/spec/features/merge_request/user_expands_diff_spec.rb
+++ b/spec/features/merge_request/user_expands_diff_spec.rb
@@ -7,7 +7,7 @@
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 @@
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
diff --git a/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb
index ffb9694e806596a05fc15ed9c306f6a8e392e6f3..7d67cde4bbb9d1d93ee2b023f38a2d758cd950b7 100644
--- a/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb
+++ b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb
@@ -15,16 +15,14 @@
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 @@
# 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 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb
index 76431d04e72ff4f31ac998e4b8a0ec420dd1d811..d803aec58950d2811a74ba406a50f97f56843922 100644
--- a/spec/features/merge_request/user_posts_diff_notes_spec.rb
+++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb
@@ -29,54 +29,54 @@
context 'with an old line on the left and no line on the right' do
it 'allows commenting on the left side' do
- should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]'), 'left')
+ should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]'), 'left')
end
it 'does not allow commenting on the right side' do
- should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right')
+ should_not_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right')
end
end
context 'with no line on the left and a new line on the right' do
it 'does not allow commenting on the left side' do
- should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left')
+ should_not_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left')
end
it 'allows commenting on the right side' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right')
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right')
end
end
context 'with an old line on the left and a new line on the right' do
it 'allows commenting on the left side', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/199050' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left')
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left')
end
it 'allows commenting on the right side' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right')
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right')
end
end
context 'with an unchanged line on the left and an unchanged line on the right' do
it 'allows commenting on the left side', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/196826' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'left')
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'left')
end
it 'allows commenting on the right side' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'right')
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'right')
end
end
context 'with a match line' do
it 'does not allow commenting' do
- line_holder = find('.match', match: :first)
+ line_holder = find_by_scrolling('.match', match: :first)
match_should_not_allow_commenting(line_holder)
end
end
context 'with an unfolded line' do
before do
- page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
+ page.within find_by_scrolling('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
find('.js-unfold', match: :first).click
end
@@ -84,12 +84,12 @@
end
it 'allows commenting on the left side' do
- should_allow_commenting(first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder [data-testid="left-side"]'))
+ should_allow_commenting(find_by_scrolling('#a5cc2925ca8258af241be7e5b0381edf30266302').first('.line_holder [data-testid="left-side"]'))
end
it 'allows commenting on the right side' do
# Automatically shifts comment box to left side.
- should_allow_commenting(first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder [data-testid="right-side"]'))
+ should_allow_commenting(find_by_scrolling('#a5cc2925ca8258af241be7e5b0381edf30266302').first('.line_holder [data-testid="right-side"]'))
end
end
end
@@ -101,44 +101,44 @@
context 'after deleteing a note' do
it 'allows commenting' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
accept_gl_confirm(button_text: 'Delete Comment') do
first('button.more-actions-toggle').click
first('.js-note-delete').click
end
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
context 'with a new line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
context 'with an old line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
+ should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
end
end
context 'with an unchanged line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
end
end
context 'with a match line' do
it 'does not allow commenting' do
- match_should_not_allow_commenting(find('.match', match: :first))
+ match_should_not_allow_commenting(find_by_scrolling('.match', match: :first))
end
end
context 'with an unfolded line' do
before do
- page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
+ page.within find_by_scrolling('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
find('.js-unfold', match: :first).click
end
@@ -147,7 +147,7 @@
# The first `.js-unfold` unfolds upwards, therefore the first
# `.line_holder` will be an unfolded line.
- let(:line_holder) { first('[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') }
+ let(:line_holder) { find_by_scrolling('[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') }
it 'allows commenting' do
should_allow_commenting line_holder
@@ -157,7 +157,7 @@
context 'when hovering over a diff discussion' do
before do
visit diffs_project_merge_request_path(project, merge_request, view: 'inline')
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
visit project_merge_request_path(project, merge_request)
end
@@ -174,7 +174,7 @@
context 'with a new line' do
it 'allows dismissing a comment' do
- should_allow_dismissing_a_comment(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ should_allow_dismissing_a_comment(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
end
@@ -182,13 +182,13 @@
describe 'with multiple note forms' do
before do
visit diffs_project_merge_request_path(project, merge_request, view: 'inline')
- click_diff_line(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
- click_diff_line(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
+ click_diff_line(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ click_diff_line(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
end
describe 'posting a note' do
it 'adds as discussion' do
- should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'), asset_form_reset: false)
+ should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'), asset_form_reset: false)
expect(page).to have_css('.notes_holder .note.note-discussion', count: 1)
expect(page).to have_field('Reply…')
end
@@ -203,25 +203,25 @@
context 'with a new line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
context 'with an old line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
+ should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
end
end
context 'with an unchanged line' do
it 'allows commenting' do
- should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
+ should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
end
end
context 'with a match line' do
it 'does not allow commenting' do
- match_should_not_allow_commenting(find('.match', match: :first))
+ match_should_not_allow_commenting(find_by_scrolling('.match', match: :first))
end
end
end
diff --git a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb
index 261d1ad325bd6e7100f482bb1db8f27159e4c9ac..33c5a936b8d2c67853faf54e3f12e9d12c30d077 100644
--- a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb
+++ b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb
@@ -6,6 +6,7 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb
index 5e022598ac3319de6b6dfdf557cb7463904f4352..7cd9ef80874201964fcaa2b7798c5b4621c7269d 100644
--- a/spec/features/merge_request/user_sees_diff_spec.rb
+++ b/spec/features/merge_request/user_sees_diff_spec.rb
@@ -5,6 +5,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb
index 5abf4e2f5adb795129cc12d077e1a5cac164276b..2b856811e0262022a131225bc70a8aa4498c3fd8 100644
--- a/spec/features/merge_request/user_sees_versions_spec.rb
+++ b/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 @@
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
diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
index 690a292937ab55a6ef03a176ec683998096d00b6..beb658bb7a0b4495ee585ba2641fd4b74f4ebc00 100644
--- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
+++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
@@ -34,7 +34,7 @@ def expect_appliable_suggestions(amount)
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 @@ def expect_appliable_suggestions(amount)
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 @@ def expect_appliable_suggestions(amount)
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 @@ def expect_appliable_suggestions(amount)
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 @@ def hash(path)
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 @@ def hash(path)
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```")
diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js
index dda758898e3511d14e7c376dce327a65cc2747d6..76e4a944d87d3627daedf96a250e2ea03d8d3321 100644
--- a/spec/frontend/diffs/components/app_spec.js
+++ b/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,
+ },
});
}
diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js
index c53789b480095d7c3cd036a593eb0900cc656a92..d6a2aa104cdc13e427b80e8551d65c13af4900b8 100644
--- a/spec/frontend/diffs/store/actions_spec.js
+++ b/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 },
diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js
index 1f71947b3cdd01034165298dde2d679165f59de8..aba80789a013b1310c9b682d4cfeecaae1e13f3c 100644
--- a/spec/frontend/notes/mixins/discussion_navigation_spec.js
+++ b/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,
});
},
);
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index f823683c274b234d54704070b2266d75618752f8..37e9ef1d994ed952a5e2b8e3c0e4f5420d9eba10 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -294,8 +294,6 @@
# 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)
diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb
index 30afde7efede6baa4eefd544ffc6130ef4de1590..7515c789addeb8bd182d227d93876491fcd077b1 100644
--- a/spec/support/helpers/merge_request_diff_helpers.rb
+++ b/spec/support/helpers/merge_request_diff_helpers.rb
@@ -1,8 +1,11 @@
# 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 @@ def get_parallel_line_components(line_holder, diff_side = nil)
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
diff --git a/spec/support/helpers/note_interaction_helpers.rb b/spec/support/helpers/note_interaction_helpers.rb
index a4322618cd35768e5280cd45bb46d016bb30983f..fa2705a64fa13fbd1510f5a887d50898c9ff1277 100644
--- a/spec/support/helpers/note_interaction_helpers.rb
+++ b/spec/support/helpers/note_interaction_helpers.rb
@@ -1,8 +1,10 @@
# 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)
diff --git a/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb b/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb
index e0a032b1a43d57cd2cdd5eacce08ebac27746c66..8a07e52019cb75e4492f8f85fec74da0e1aaa098 100644
--- a/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb
+++ b/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')