From 0668179c9aaba492b58c693280b6ad80e8998331 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Feb 2020 12:50:48 -0700 Subject: [PATCH 1/6] Always return the existing files when loading batched diffs Other than utils.js, the changes here are just to change the prepareDiffData arguments to use an object. In utils.js, I've just updated the logic to do a more robust merge + blend to accommodate pages of batched diff data loading over existing state. --- app/assets/javascripts/diffs/store/actions.js | 4 +- .../javascripts/diffs/store/mutations.js | 4 +- app/assets/javascripts/diffs/store/utils.js | 75 +++++++++++++------ spec/javascripts/diffs/store/utils_spec.js | 39 +++++++++- 4 files changed, 93 insertions(+), 29 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index b920e0411358e1..e61fd3e7b6f4c9 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -150,8 +150,8 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []); commit(types.SET_DIFF_DATA, strippedData); - prepareDiffData(data); - worker.postMessage(data.diff_files); + worker.postMessage(prepareDiffData(data, state.diffFiles)); + return data; }) .catch(() => worker.terminate()); diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 1505be1a0b2f8e..c26411af5d73a2 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -148,8 +148,8 @@ export default { }, [types.ADD_COLLAPSED_DIFFS](state, { file, data }) { - prepareDiffData(data); - const [newFileData] = data.diff_files.filter(f => f.file_hash === file.file_hash); + const files = prepareDiffData(data); + const [newFileData] = files.filter(f => f.file_hash === file.file_hash); const selectedFile = state.diffFiles.find(f => f.file_hash === file.file_hash); Object.assign(selectedFile, { ...newFileData }); }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index b379f1fabef839..0f2ca0557604d8 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -217,30 +217,50 @@ function diffFileUniqueId(file) { return `${file.content_sha}-${file.file_hash}`; } -function combineDiffFilesWithPriorFiles(files, prior = []) { - files.forEach(file => { - const id = diffFileUniqueId(file); - const oldMatch = prior.find(oldFile => diffFileUniqueId(oldFile) === id); +function matchFileToListOfFiles(list, file) { + return list.find(matched => diffFileUniqueId(matched) === diffFileUniqueId(file)); +} - if (oldMatch) { - const missingInline = !file.highlighted_diff_lines; - const missingParallel = !file.parallel_diff_lines; +function blendTwoFiles(authoritativeFile, overwritingFile) { + const missingInline = !authoritativeFile.highlighted_diff_lines; + const missingParallel = !authoritativeFile.parallel_diff_lines; - if (missingInline) { - Object.assign(file, { - highlighted_diff_lines: oldMatch.highlighted_diff_lines, - }); - } + if (missingInline) { + Object.assign(authoritativeFile, { + highlighted_diff_lines: overwritingFile.highlighted_diff_lines, + }); + } + + if (missingParallel) { + Object.assign(authoritativeFile, { + parallel_diff_lines: overwritingFile.parallel_diff_lines, + }); + } + + return authoritativeFile; +} - if (missingParallel) { - Object.assign(file, { - parallel_diff_lines: oldMatch.parallel_diff_lines, - }); +function combineDiffFilesWithPriorFiles(files, prior = [], batched) { + const authoritative = batched ? prior : files; + const overwrite = batched ? files : prior; + + if (batched) { + files.forEach(newFile => { + const foundFile = matchFileToListOfFiles(authoritative, newFile); + if (!foundFile) { + authoritative.push(newFile); + } else { + blendTwoFiles(foundFile, newFile); } - } - }); + }); + } else { + authoritative.forEach(file => { + const matched = matchFileToListOfFiles(overwrite, file) || {}; + blendTwoFiles(file, matched); + }); + } - return files; + return authoritative; } function ensureBasicDiffFileLines(file) { @@ -318,8 +338,21 @@ function finalizeDiffFile(file) { return file; } -export function prepareDiffData(diffData, priorFiles) { - return combineDiffFilesWithPriorFiles(diffData.diff_files, priorFiles) +function deduplicateFilesList(files) { + const dedupedFiles = files.reduce(( newList, file ) => { + const id = diffFileUniqueId(file); + + return { + ...newList, + [id]: newList[id] ? mergeTwoFiles(newList[id], file) : file + }; + }, {} ); + + return Object.values(dedupedFiles); +} + +export function prepareDiffData(diff, priorFiles = []) { + const cleanedFiles = (diff.diff_files || []) .map(ensureBasicDiffFileLines) .map(prepareDiffFileLines) .map(finalizeDiffFile); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 638b4510221692..051820cedfa15c 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -333,10 +333,10 @@ describe('DiffsStoreUtils', () => { diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })], }; - utils.prepareDiffData(preparedDiff); - utils.prepareDiffData(splitInlineDiff); - utils.prepareDiffData(splitParallelDiff); - utils.prepareDiffData(completedDiff, [mock]); + preparedDiff.diff_files = utils.prepareDiffData(preparedDiff); + splitInlineDiff.diff_files = utils.prepareDiffData(splitInlineDiff); + splitParallelDiff.diff_files = utils.prepareDiffData(splitParallelDiff); + completedDiff.diff_files = utils.prepareDiffData(completedDiff, [mock]); }); it('sets the renderIt and collapsed attribute on files', () => { @@ -390,6 +390,37 @@ describe('DiffsStoreUtils', () => { expect(completedDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0); expect(completedDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0); }); + + it('leaves files in the existing state', () => { + const priorFiles = [mock]; + const fakeNewFile = { + ...mock, + content_sha: 'ABC', + file_hash: 'DEF', + }; + const updatedFilesList = utils.prepareDiffData({ diff_files: [fakeNewFile] }, priorFiles); + + expect(updatedFilesList).toEqual([mock, fakeNewFile]); + }); + + it('completes an existing split diff without overwriting existing diffs', () => { + // The current state has a file that has only loaded inline lines + const priorFiles = [{ ...mock, parallel_diff_lines: [] }]; + // The next (batch) load loads two files: the other half of that file, and a new file + const fakeBatch = [ + { ...mock, highlighted_diff_lines: undefined }, + { ...mock, highlighted_diff_lines: undefined, content_sha: 'ABC', file_hash: 'DEF' }, + ]; + const updatedFilesList = utils.prepareDiffData({ diff_files: fakeBatch }, priorFiles); + + expect(updatedFilesList).toEqual([ + mock, + jasmine.objectContaining({ + content_sha: 'ABC', + file_hash: 'DEF', + }), + ]); + }); }); describe('isDiscussionApplicableToLine', () => { -- GitLab From 3bd95adf74b2d8465731f2f9d3d523e2c2eeea26 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Feb 2020 01:42:01 -0700 Subject: [PATCH 2/6] Test batch + split diff file loads --- spec/javascripts/diffs/store/mutations_spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 24405dcc7968cd..cb89a89e2165fb 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -55,8 +55,8 @@ describe('DiffsStoreMutations', () => { const state = { diffFiles: [ { - content_sha: diffFileMockData.content_sha, - file_hash: diffFileMockData.file_hash, + ...diffFileMockData, + parallel_diff_lines: [], }, ], }; -- GitLab From 73130e248cbaee6011ada6e250adeea22bb3b505 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Feb 2020 13:32:16 -0700 Subject: [PATCH 3/6] Clean up and simplify loading diff files - Newly loaded files are always added to the end and deduped - Deduping is done last - Lines aren't prepared after they've been prepped once --- app/assets/javascripts/diffs/store/utils.js | 90 ++++++++------------- 1 file changed, 32 insertions(+), 58 deletions(-) diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 0f2ca0557604d8..80972d2aeb8be3 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -217,50 +217,19 @@ function diffFileUniqueId(file) { return `${file.content_sha}-${file.file_hash}`; } -function matchFileToListOfFiles(list, file) { - return list.find(matched => diffFileUniqueId(matched) === diffFileUniqueId(file)); -} - -function blendTwoFiles(authoritativeFile, overwritingFile) { - const missingInline = !authoritativeFile.highlighted_diff_lines; - const missingParallel = !authoritativeFile.parallel_diff_lines; - - if (missingInline) { - Object.assign(authoritativeFile, { - highlighted_diff_lines: overwritingFile.highlighted_diff_lines, - }); - } - - if (missingParallel) { - Object.assign(authoritativeFile, { - parallel_diff_lines: overwritingFile.parallel_diff_lines, - }); - } +function mergeTwoFiles(target, source) { + const originalInline = target.highlighted_diff_lines; + const originalParallel = target.parallel_diff_lines; + const missingInline = !originalInline.length; + const missingParallel = !originalParallel.length; - return authoritativeFile; -} - -function combineDiffFilesWithPriorFiles(files, prior = [], batched) { - const authoritative = batched ? prior : files; - const overwrite = batched ? files : prior; - - if (batched) { - files.forEach(newFile => { - const foundFile = matchFileToListOfFiles(authoritative, newFile); - if (!foundFile) { - authoritative.push(newFile); - } else { - blendTwoFiles(foundFile, newFile); - } - }); - } else { - authoritative.forEach(file => { - const matched = matchFileToListOfFiles(overwrite, file) || {}; - blendTwoFiles(file, matched); - }); - } - - return authoritative; + return { + ...target, + highlighted_diff_lines: missingInline ? source.highlighted_diff_lines : originalInline, + parallel_diff_lines: missingParallel ? source.parallel_diff_lines : originalParallel, + renderIt: source.renderIt, + collapsed: source.collapsed, + }; } function ensureBasicDiffFileLines(file) { @@ -280,13 +249,16 @@ function cleanRichText(text) { } function prepareLine(line) { - return Object.assign(line, { - rich_text: cleanRichText(line.rich_text), - discussionsExpanded: true, - discussions: [], - hasForm: false, - text: undefined, - }); + if (!line.alreadyPrepared) { + Object.assign(line, { + rich_text: cleanRichText(line.rich_text), + discussionsExpanded: true, + discussions: [], + hasForm: false, + text: undefined, + alreadyPrepared: true, + }); + } } function prepareDiffFileLines(file) { @@ -308,11 +280,11 @@ function prepareDiffFileLines(file) { parallelLinesCount += 1; prepareLine(line.right); } + }); - Object.assign(file, { - inlineLinesCount: inlineLines.length, - parallelLinesCount, - }); + Object.assign(file, { + inlineLinesCount: inlineLines.length, + parallelLinesCount, }); return file; @@ -339,14 +311,14 @@ function finalizeDiffFile(file) { } function deduplicateFilesList(files) { - const dedupedFiles = files.reduce(( newList, file ) => { + const dedupedFiles = files.reduce((newList, file) => { const id = diffFileUniqueId(file); - + return { ...newList, - [id]: newList[id] ? mergeTwoFiles(newList[id], file) : file + [id]: newList[id] ? mergeTwoFiles(newList[id], file) : file, }; - }, {} ); + }, {}); return Object.values(dedupedFiles); } @@ -356,6 +328,8 @@ export function prepareDiffData(diff, priorFiles = []) { .map(ensureBasicDiffFileLines) .map(prepareDiffFileLines) .map(finalizeDiffFile); + + return deduplicateFilesList([...priorFiles, ...cleanedFiles]); } export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) { -- GitLab From be7b37379803399381d6a7169dd7ad6efd28470a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Feb 2020 03:00:10 -0700 Subject: [PATCH 4/6] Make sure the endpoints are always requesting the right view style Also: pass the current state in when computing the tree data --- app/assets/javascripts/diffs/store/actions.js | 21 +++- spec/javascripts/diffs/store/actions_spec.js | 117 +++++++++++++++++- 2 files changed, 128 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index e61fd3e7b6f4c9..3b65509a74caf5 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -111,15 +111,22 @@ export const fetchDiffFilesBatch = ({ commit, state }) => { commit(types.SET_BATCH_LOADING, true); commit(types.SET_RETRIEVING_BATCHES, true); - const getBatch = page => + const getBatch = (page = 1) => axios .get(state.endpointBatch, { - params: { ...urlParams, page }, + params: { + ...urlParams, + page, + }, }) .then(({ data: { pagination, diff_files } }) => { commit(types.SET_DIFF_DATA_BATCH, { diff_files }); commit(types.SET_BATCH_LOADING, false); - if (!pagination.next_page) commit(types.SET_RETRIEVING_BATCHES, false); + + if (!pagination.next_page) { + commit(types.SET_RETRIEVING_BATCHES, false); + } + return pagination.next_page; }) .then(nextPage => nextPage && getBatch(nextPage)) @@ -132,6 +139,11 @@ export const fetchDiffFilesBatch = ({ commit, state }) => { export const fetchDiffFilesMeta = ({ commit, state }) => { const worker = new TreeWorker(); + const urlParams = {}; + + if (state.useSingleDiffStyle) { + urlParams.view = state.diffViewType; + } commit(types.SET_LOADING, true); @@ -142,9 +154,10 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { }); return axios - .get(state.endpointMetadata) + .get(mergeUrlParams(urlParams, state.endpointMetadata)) .then(({ data }) => { const strippedData = { ...data }; + delete strippedData.diff_files; commit(types.SET_LOADING, false); commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index af2dd7b4f93c2a..ff17d8ec158023 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -158,16 +158,19 @@ describe('DiffsStoreActions', () => { const res1 = { diff_files: [], pagination: { next_page: 2 } }; const res2 = { diff_files: [], pagination: {} }; mock - .onGet(endpointBatch, { params: { page: undefined, per_page: DIFFS_PER_PAGE, w: '1' } }) - .reply(200, res1); - mock - .onGet(endpointBatch, { params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1' } }) + .onGet(endpointBatch, { + params: { page: 1, per_page: DIFFS_PER_PAGE, w: '1', view: 'inline' }, + }) + .reply(200, res1) + .onGet(endpointBatch, { + params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1', view: 'inline' }, + }) .reply(200, res2); testAction( fetchDiffFilesBatch, {}, - { endpointBatch }, + { endpointBatch, useSingleDiffStyle: true, diffViewType: 'inline' }, [ { type: types.SET_BATCH_LOADING, payload: true }, { type: types.SET_RETRIEVING_BATCHES, payload: true }, @@ -188,7 +191,7 @@ describe('DiffsStoreActions', () => { describe('fetchDiffFilesMeta', () => { it('should fetch diff meta information', done => { - const endpointMetadata = '/fetch/diffs_meta'; + const endpointMetadata = '/fetch/diffs_meta?view=inline'; const mock = new MockAdapter(axios); const data = { diff_files: [] }; const res = { data }; @@ -213,6 +216,108 @@ describe('DiffsStoreActions', () => { }); }); + describe('when the single diff view feature flag is off', () => { + describe('fetchDiffFiles', () => { + it('should fetch diff files', done => { + const endpoint = '/fetch/diff/files?w=1'; + const mock = new MockAdapter(axios); + const res = { diff_files: 1, merge_request_diffs: [] }; + mock.onGet(endpoint).reply(200, res); + + testAction( + fetchDiffFiles, + {}, + { + endpoint, + diffFiles: [], + showWhitespace: false, + diffViewType: 'inline', + useSingleDiffStyle: false, + }, + [ + { type: types.SET_LOADING, payload: true }, + { type: types.SET_LOADING, payload: false }, + { type: types.SET_MERGE_REQUEST_DIFFS, payload: res.merge_request_diffs }, + { type: types.SET_DIFF_DATA, payload: res }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + + fetchDiffFiles({ state: { endpoint }, commit: () => null }) + .then(data => { + expect(data).toEqual(res); + done(); + }) + .catch(done.fail); + }); + }); + + describe('fetchDiffFilesBatch', () => { + it('should fetch batch diff files', done => { + const endpointBatch = '/fetch/diffs_batch'; + const mock = new MockAdapter(axios); + const res1 = { diff_files: [], pagination: { next_page: 2 } }; + const res2 = { diff_files: [], pagination: {} }; + mock + .onGet(endpointBatch, { params: { page: 1, per_page: DIFFS_PER_PAGE, w: '1' } }) + .reply(200, res1) + .onGet(endpointBatch, { params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1' } }) + .reply(200, res2); + + testAction( + fetchDiffFilesBatch, + {}, + { endpointBatch, useSingleDiffStyle: false }, + [ + { type: types.SET_BATCH_LOADING, payload: true }, + { type: types.SET_RETRIEVING_BATCHES, payload: true }, + { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } }, + { type: types.SET_BATCH_LOADING, payload: false }, + { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: [] } }, + { type: types.SET_BATCH_LOADING, payload: false }, + { type: types.SET_RETRIEVING_BATCHES, payload: false }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + }); + }); + + describe('fetchDiffFilesMeta', () => { + it('should fetch diff meta information', done => { + const endpointMetadata = '/fetch/diffs_meta?'; + const mock = new MockAdapter(axios); + const data = { diff_files: [] }; + const res = { data }; + mock.onGet(endpointMetadata).reply(200, res); + + testAction( + fetchDiffFilesMeta, + {}, + { endpointMetadata, useSingleDiffStyle: false }, + [ + { type: types.SET_LOADING, payload: true }, + { type: types.SET_LOADING, payload: false }, + { type: types.SET_MERGE_REQUEST_DIFFS, payload: [] }, + { type: types.SET_DIFF_DATA, payload: { data } }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + }); + }); + }); + describe('setHighlightedRow', () => { it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => { testAction(setHighlightedRow, 'ABC_123', {}, [ -- GitLab From 4d35ff36729ed5dd9f33ed9398f5df1edf8f52d0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 3 Feb 2020 21:55:30 -0700 Subject: [PATCH 5/6] Fix string comparison It probably shouldn't be comparing a boolean to a string --- app/assets/javascripts/diffs/store/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 3b65509a74caf5..bd85105ccb4c71 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -239,7 +239,7 @@ export const startRenderDiffsQueue = ({ state, commit }) => { const nextFile = state.diffFiles.find( file => !file.renderIt && - (file.viewer && (!file.viewer.collapsed || !file.viewer.name === diffViewerModes.text)), + (file.viewer && (!file.viewer.collapsed || file.viewer.name !== diffViewerModes.text)), ); if (nextFile) { -- GitLab From 381fbc89a449aa179f048e428267995db4472c43 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 6 Feb 2020 14:01:20 -0700 Subject: [PATCH 6/6] Add RSpec regression tests --- ...er_interacts_with_batched_mr_diffs_spec.rb | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb 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 new file mode 100644 index 00000000000000..92d90926c0aeec --- /dev/null +++ b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Batch diffs', :js do + include MergeRequestDiffHelpers + include RepoHelpers + + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'empty-branch') } + + before do + stub_feature_flags(single_mr_diff_view: true) + stub_feature_flags(diffs_batch_load: true) + + sign_in(project.owner) + + 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 tr.line_holder.new:first-of-type')) + page.within('.js-discussion-note-form') do + fill_in('note_note', with: 'First Line Comment') + click_button('Comment') + end + + # Add discussion to first line of last file + click_diff_line(find('.diff-file.file-holder:last-of-type tr.line_holder.new:first-of-type')) + page.within('.js-discussion-note-form') do + fill_in('note_note', with: 'Last Line Comment') + click_button('Comment') + end + + wait_for_requests + end + + it 'assigns discussions to diff files across multiple batch pages' 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 + 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 + expect(page).to have_content('Last Line Comment') + end + end + + 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 + click_link('just now') + end + + visit page.current_url + + 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 + end + end + + context 'which is in at least page 2 of the batched pages of diffs' do + it 'scrolls to the correct discussion' do + page.within('.diff-file.file-holder:last-of-type') do + click_link('just now') + end + + visit page.current_url + + 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 + end + end + end + + context 'when user switches view styles' do + before do + find('.js-show-diff-settings').click + click_button 'Side-by-side' + + wait_for_requests + 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 + 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 + expect(page).to have_content('Last Line Comment') + end + end + end +end -- GitLab