From 9e39ded3fc89a296670c386c45d71c27d945a836 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:08:20 -0700 Subject: [PATCH 01/12] Add flag to not manipulate the URL in some cases --- app/assets/javascripts/merge_request_tabs.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 5a1410ceeba328..f868cad7c161fa 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -135,7 +135,7 @@ function destroyPipelines(app) { } function loadDiffs({ url, sticky }) { - return axios.get(`${url}.json${location.search}`).then(({ data }) => { + return axios.get(url).then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); initDiffStatsDropdown(sticky); @@ -341,7 +341,7 @@ export default class MergeRequestTabs { in practice, this only occurs when comparing commits in the new merge request form page. */ - this.loadDiff(href); + this.loadDiff({ endpoint: href, strip: true }); } // this.hideSidebar(); this.expandViewContainer(); @@ -503,16 +503,18 @@ export default class MergeRequestTabs { } // load the diff tab content from the backend - loadDiff(source) { + loadDiff({ endpoint, strip = true }) { if (this.diffsLoaded) { document.dispatchEvent(new CustomEvent('scroll')); return; } + const diffUrl = strip ? `${parseUrlPathname(endpoint)}.json${location.search}` : endpoint; + loadDiffs({ // We extract pathname for the current Changes tab anchor href // some pages like MergeRequestsController#new has query parameters on that anchor - url: parseUrlPathname(source), + url: diffUrl, sticky: computeTopOffset(this.mergeRequestTabs), }) .then(() => { -- GitLab From 39de7b5b8f858ba292bfa8276024e283e3e0b717 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:49:20 -0700 Subject: [PATCH 02/12] Only construct the Diff class when the diffs are loaded --- .../pages/projects/merge_requests/init_merge_request.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js index a4e3ddfc50623f..d4734b8842dfb7 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js +++ b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js @@ -4,14 +4,12 @@ import $ from 'jquery'; import IssuableForm from 'ee_else_ce/issuable/issuable_form'; import IssuableLabelSelector from '~/issuable/issuable_label_selector'; import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation'; -import Diff from '~/diff'; import GLForm from '~/gl_form'; import LabelsSelect from '~/labels/labels_select'; import IssuableTemplateSelectors from '~/issuable/issuable_template_selectors'; import { mountMilestoneDropdown } from '~/sidebar/mount_sidebar'; export default () => { - new Diff(); new ShortcutsNavigation(); new GLForm($('.merge-request-form')); new IssuableForm($('.merge-request-form')); -- GitLab From 7c96bbc4c133d19ce8fffb68ffdbaf3dafa34f89 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:50:12 -0700 Subject: [PATCH 03/12] Only construct the Diff class once --- app/assets/javascripts/merge_request_tabs.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index f868cad7c161fa..fd7faa763feb48 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -143,7 +143,10 @@ function loadDiffs({ url, sticky }) { localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); syntaxHighlight($('#diffs .js-syntax-highlight')); - new Diff(); + if (!tabs.diffsClass) { + tabs.diffsClass = new Diff(); + } + scrollToContainer('#diffs'); $('.diff-file').each((i, el) => { @@ -204,6 +207,7 @@ export default class MergeRequestTabs { this.currentTab = null; this.diffsLoaded = false; + this.diffsClass = null; this.commitsLoaded = false; this.fixedLayoutPref = null; this.eventHub = createEventHub(); -- GitLab From 146d323e58feff54921f8489699c866de14c5075 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:51:10 -0700 Subject: [PATCH 04/12] Accept the Merge Request (Tabs) event hub when constructing the Diff --- app/assets/javascripts/diff.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 23eb470503e8a5..336b8c2c49f771 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -12,9 +12,13 @@ const UNFOLD_COUNT = 20; let isBound = false; export default class Diff { - constructor() { + constructor({ mergeRequestEventHub } = {}) { const $diffFile = $('.files .diff-file'); + if (mergeRequestEventHub) { + this.mrHub = mergeRequestEventHub; + } + $diffFile.each((index, file) => { if (!$.data(file, 'singleFileDiff')) { $.data(file, 'singleFileDiff', new SingleFileDiff(file)); -- GitLab From 53d927c43bfe3804981b4cc4075b729ace514483 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:52:25 -0700 Subject: [PATCH 05/12] Emit an event if the Diff has an MR Event Hub This is instead of the normal link click behavior, which would refresh the page. --- app/assets/javascripts/diff.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 336b8c2c49f771..6581649543253c 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -38,7 +38,8 @@ export default class Diff { $(document) .on('click', '.js-unfold', this.handleClickUnfold.bind(this)) .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this)) - .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this)); + .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this)) + .on('click', '.inline-parallel-buttons a', ($e) => this.viewTypeSwitch($e)); isBound = true; } @@ -139,6 +140,20 @@ export default class Diff { diffViewType() { return $('.inline-parallel-buttons a.active').data('viewType'); } + viewTypeSwitch(event) { + const click = event.originalEvent; + const diffSource = new URL(click.target.getAttribute('href'), document.location.href); + + if (this.mrHub) { + click.preventDefault(); + click.stopPropagation(); + + diffSource.pathname = `${diffSource.pathname}.json`; + + this.mrHub.$emit('diff:switch-view-type', { source: diffSource.toString() }); + } + } + // eslint-disable-next-line class-methods-use-this lineNumbers(line) { const children = line.find('.diff-line-num').toArray(); -- GitLab From 0886d522ff5a7c19e6695af60b62c21f7f011a24 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:56:10 -0700 Subject: [PATCH 06/12] Provide the eventHub to the Diff --- app/assets/javascripts/merge_request_tabs.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index fd7faa763feb48..7f5252f0d3b655 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -134,7 +134,7 @@ function destroyPipelines(app) { return null; } -function loadDiffs({ url, sticky }) { +function loadDiffs({ url, sticky, tabs }) { return axios.get(url).then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); @@ -144,7 +144,9 @@ function loadDiffs({ url, sticky }) { syntaxHighlight($('#diffs .js-syntax-highlight')); if (!tabs.diffsClass) { - tabs.diffsClass = new Diff(); + tabs.diffsClass = new Diff({ mergeRequestEventHub: tabs.eventHub }); + } else { + tabs.diffsClass.mrHub = tabs.eventHub; } scrollToContainer('#diffs'); @@ -520,6 +522,7 @@ export default class MergeRequestTabs { // some pages like MergeRequestsController#new has query parameters on that anchor url: diffUrl, sticky: computeTopOffset(this.mergeRequestTabs), + tabs: this, }) .then(() => { if (this.isDiffAction(this.currentAction)) { -- GitLab From c2ada943399e14e307fd82314ca7b13a41ba9eb6 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 17:56:49 -0700 Subject: [PATCH 07/12] Load switched diff view type asynchronously The real commit message: "Handle an eventHub request to switch the view type" The commit message in the subject of this commit is vague and not descriptive because it's what's used by the Changelog and none of the commits in this branch would be appropriate for a Changelog, so I had to pick the _most_ relevant one to make the Changelog entry. Changelog: changed --- app/assets/javascripts/merge_request_tabs.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 7f5252f0d3b655..587bb23b34304a 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -217,6 +217,7 @@ export default class MergeRequestTabs { this.setUrl = setUrl !== undefined ? setUrl : true; this.setCurrentAction = this.setCurrentAction.bind(this); + this.switchViewType = this.switchViewType.bind(this); this.tabShown = this.tabShown.bind(this); this.clickTab = this.clickTab.bind(this); @@ -236,11 +237,13 @@ export default class MergeRequestTabs { this.tabShown(action, location.href); this.eventHub.$emit('MergeRequestTabChange', action); }); + this.eventHub.$on('diff:switch-view-type', this.switchViewType); } // Used in tests unbindEvents() { $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab); + this.eventHub.$off('diff:switch-view-type', this.switchViewType); } storeScroll() { @@ -537,6 +540,11 @@ export default class MergeRequestTabs { }); }); } + switchViewType({ source }) { + this.diffsLoaded = false; + + this.loadDiff({ endpoint: source, strip: false }); + } diffViewType() { return $('.js-diff-view-buttons button.active').data('viewType'); -- GitLab From 4ea6cf1d928ede53cd9525853b93a5fac127b5b0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 15 Dec 2022 18:13:48 -0700 Subject: [PATCH 08/12] Don't leave a placeholder when sticking the files-changed bar This bar isn't actually stuck anyway ?????? Not calling the method causes issues, but it doesn't stick when you scroll, so toggling off this placeholder just prevents an error with events trying to use DOM nodes that aren't there any more now that the DOM is changing out from beneath the management script. --- app/assets/javascripts/init_diff_stats_dropdown.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/init_diff_stats_dropdown.js b/app/assets/javascripts/init_diff_stats_dropdown.js index 27df761a10308f..8413fe92f8948f 100644 --- a/app/assets/javascripts/init_diff_stats_dropdown.js +++ b/app/assets/javascripts/init_diff_stats_dropdown.js @@ -4,7 +4,7 @@ import { stickyMonitor } from './lib/utils/sticky'; export const initDiffStatsDropdown = (stickyTop) => { if (stickyTop) { - stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop); + stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop, false); } const el = document.querySelector('.js-diff-stats-dropdown'); -- GitLab From 8cc9881c3890adefbbe715a81460e600d98b3aef Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 16 Dec 2022 12:57:29 -0700 Subject: [PATCH 09/12] Remove unnecessary ESLint disablements --- app/assets/javascripts/merge_request_tabs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 587bb23b34304a..f3868b83c16021 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -1,4 +1,4 @@ -/* eslint-disable no-new, class-methods-use-this */ +/* eslint-disable class-methods-use-this */ import $ from 'jquery'; import Vue from 'vue'; import { createAlert } from '~/flash'; -- GitLab From d1bad28aa4c1055186f3a46881e5d02f5e009ed7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 16 Dec 2022 12:58:03 -0700 Subject: [PATCH 10/12] Call a function rather than setting to a function parameter --- app/assets/javascripts/merge_request_tabs.js | 21 +++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index f3868b83c16021..46ee8fecfc5736 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -143,11 +143,8 @@ function loadDiffs({ url, sticky, tabs }) { localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); syntaxHighlight($('#diffs .js-syntax-highlight')); - if (!tabs.diffsClass) { - tabs.diffsClass = new Diff({ mergeRequestEventHub: tabs.eventHub }); - } else { - tabs.diffsClass.mrHub = tabs.eventHub; - } + tabs.createDiff(); + tabs.setHubToDiff(); scrollToContainer('#diffs'); @@ -518,11 +515,11 @@ export default class MergeRequestTabs { return; } + // We extract pathname for the current Changes tab anchor href + // some pages like MergeRequestsController#new has query parameters on that anchor const diffUrl = strip ? `${parseUrlPathname(endpoint)}.json${location.search}` : endpoint; loadDiffs({ - // We extract pathname for the current Changes tab anchor href - // some pages like MergeRequestsController#new has query parameters on that anchor url: diffUrl, sticky: computeTopOffset(this.mergeRequestTabs), tabs: this, @@ -545,6 +542,16 @@ export default class MergeRequestTabs { this.loadDiff({ endpoint: source, strip: false }); } + createDiff() { + if (!this.diffsClass) { + this.diffsClass = new Diff({ mergeRequestEventHub: this.eventHub }); + } + } + setHubToDiff() { + if (this.diffsClass) { + this.diffsClass.mrHub = this.eventHub; + } + } diffViewType() { return $('.js-diff-view-buttons button.active').data('viewType'); -- GitLab From 34783cc8dd01047eb64209a461b4d89982be4770 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 16 Dec 2022 18:09:37 -0700 Subject: [PATCH 11/12] Test new tabs behaviors interacting with Diffs Notably, because this class ONLY works with some methods `.bind`ed to `this` and then re-set to the class, we can't technically test that when it receives the new event for switching view types, it actually calls the class method. This is wild. We could technically test that the sub-call into `loadDiff` is made but that's not a direct test. I've opted to simply not test the eventHub -> method call pipeline because HOPEFULLY the event hub is working (testing the call would just test that the hub is functioning) and we can test that the event is emitted from the Diff side and consider it done. --- spec/frontend/merge_request_tabs_spec.js | 70 ++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 69ff5e47689849..6d434d7e654d3e 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -5,6 +5,7 @@ import initMrPage from 'helpers/init_vue_mr_page_helper'; import { stubPerformanceWebAPI } from 'helpers/performance'; import axios from '~/lib/utils/axios_utils'; import MergeRequestTabs from '~/merge_request_tabs'; +import Diff from '~/diff'; import '~/lib/utils/common_utils'; import '~/lib/utils/url_utility'; @@ -389,4 +390,73 @@ describe('MergeRequestTabs', () => { }); }); }); + + describe('tabs <-> diff interactions', () => { + beforeEach(() => { + jest.spyOn(testContext.class, 'loadDiff').mockImplementation(() => {}); + }); + + describe('switchViewType', () => { + it('marks the class as having not loaded diffs already', () => { + testContext.class.diffsLoaded = true; + + testContext.class.switchViewType({}); + + expect(testContext.class.diffsLoaded).toBe(false); + }); + + it('reloads the diffs', () => { + testContext.class.switchViewType({ source: 'a new url' }); + + expect(testContext.class.loadDiff).toHaveBeenCalledWith({ + endpoint: 'a new url', + strip: false, + }); + }); + }); + + describe('createDiff', () => { + it("creates a Diff if there isn't one", () => { + expect(testContext.class.diffsClass).toBe(null); + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass).toBeInstanceOf(Diff); + }); + + it("doesn't create a Diff if one already exists", () => { + testContext.class.diffsClass = 'truthy'; + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass).toBe('truthy'); + }); + + it('sets the available MR Tabs event hub to the new Diff', () => { + expect(testContext.class.diffsClass).toBe(null); + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass.mrHub).toBe(testContext.class.eventHub); + }); + }); + + describe('setHubToDiff', () => { + it('sets the MR Tabs event hub to the child Diff', () => { + testContext.class.diffsClass = {}; + + testContext.class.setHubToDiff(); + + expect(testContext.class.diffsClass.mrHub).toBe(testContext.class.eventHub); + }); + + it('does not fatal if theres no child Diff', () => { + testContext.class.diffsClass = null; + + expect(() => { + testContext.class.setHubToDiff(); + }).not.toThrow(); + }); + }); + }); }); -- GitLab From 3db150e962ca85113eb6c86bd7d83606e27e9f5b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 16 Dec 2022 19:12:56 -0700 Subject: [PATCH 12/12] Add tests for interrupted diff view type switch --- spec/frontend/diff_spec.js | 72 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 spec/frontend/diff_spec.js diff --git a/spec/frontend/diff_spec.js b/spec/frontend/diff_spec.js new file mode 100644 index 00000000000000..759ae32ac514ef --- /dev/null +++ b/spec/frontend/diff_spec.js @@ -0,0 +1,72 @@ +import createEventHub from '~/helpers/event_hub_factory'; + +import Diff from '~/diff'; + +describe('Diff', () => { + describe('diff <-> tabs interactions', () => { + let hub; + + beforeEach(() => { + hub = createEventHub(); + }); + + describe('constructor', () => { + it("takes in the `mergeRequestEventHub` when it's provided", () => { + const diff = new Diff({ mergeRequestEventHub: hub }); + + expect(diff.mrHub).toBe(hub); + }); + + it('does not fatal if no event hub is provided', () => { + expect(() => { + new Diff(); /* eslint-disable-line no-new */ + }).not.toThrow(); + }); + + it("doesn't set the mrHub property if none is provided by the construction arguments", () => { + const diff = new Diff(); + + expect(diff.mrHub).toBe(undefined); + }); + }); + + describe('viewTypeSwitch', () => { + const clickPath = '/path/somewhere?params=exist'; + const jsonPath = 'http://test.host/path/somewhere.json?params=exist'; + const simulatejQueryClick = { + originalEvent: { + target: { + getAttribute() { + return clickPath; + }, + }, + preventDefault: jest.fn(), + stopPropagation: jest.fn(), + }, + }; + + it('emits the correct switch view event when called and there is an `mrHub`', async () => { + const diff = new Diff({ mergeRequestEventHub: hub }); + const hubEmit = new Promise((resolve) => { + hub.$on('diff:switch-view-type', resolve); + }); + + diff.viewTypeSwitch(simulatejQueryClick); + const { source } = await hubEmit; + + expect(simulatejQueryClick.originalEvent.preventDefault).toHaveBeenCalled(); + expect(simulatejQueryClick.originalEvent.stopPropagation).toHaveBeenCalled(); + expect(source).toBe(jsonPath); + }); + + it('is effectively a noop when there is no `mrHub`', () => { + const diff = new Diff(); + + expect(diff.mrHub).toBe(undefined); + expect(() => { + diff.viewTypeSwitch(simulatejQueryClick); + }).not.toThrow(); + }); + }); + }); +}); -- GitLab