From a300c1f7233b5a34144c70e1c8d9c1d2b0c7a086 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 20 Mar 2024 01:20:31 -0600 Subject: [PATCH 1/3] Fix native browser navigation in MR single-file mode - Handle hash changes that occur when the Diffs app is running - Trigger the fetchFileByFile action, which loads a local version if possible Changelog: fixed --- .../javascripts/diffs/components/app.vue | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index f3b306d75910bb..38ef2fd4232da0 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -21,7 +21,7 @@ import { helpPagePath } from '~/helpers/help_page_helper'; import { parseBoolean, handleLocationHash } from '~/lib/utils/common_utils'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import { Mousetrap } from '~/lib/mousetrap'; -import { updateHistory } from '~/lib/utils/url_utility'; +import { updateHistory, getLocationHash } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import notesEventHub from '~/notes/event_hub'; @@ -379,6 +379,7 @@ export default { queueRedisHllEvents(events, { verifyCap: true }); this.subscribeToVirtualScrollingEvents(); + window.addEventListener('hashchange', this.handleHashChange); }, beforeCreate() { diffsApp.instrument(); @@ -405,6 +406,8 @@ export default { this.unsubscribeFromEvents(); this.removeEventListeners(); + window.removeEventListener('hashchange', this.handleHashChange); + diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash); diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex); }, @@ -421,6 +424,7 @@ export default { 'rereadNoteHash', 'startRenderDiffsQueue', 'assignDiscussionsToDiff', + 'setCurrentFileHash', 'setHighlightedRow', 'goToFile', 'setShowTreeList', @@ -490,6 +494,18 @@ export default { } } }, + handleHashChange() { + let hash = getLocationHash(); + + if (this.viewDiffsFileByFile) { + if (!hash) { + hash = this.diffFiles[0].file_hash; + } + + this.setCurrentFileHash(hash); + this.fetchFileByFile(); + } + }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, -- GitLab From e8ee765a1deccac3b4df30ea4e420de4eaaa79d2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 26 Mar 2024 20:33:30 -0600 Subject: [PATCH 2/3] Test how hash changes are handled in the UI --- spec/frontend/diffs/components/app_spec.js | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 22c17f02cb0f9c..745c99636cfb21 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -814,6 +814,61 @@ describe('diffs/components/app', () => { }, ); }); + + describe('non-UI navigation', () => { + let currentHash; + let fetchFbf; + + beforeEach(() => { + currentHash = jest.fn(); + fetchFbf = jest.fn(); + window.location.hash = '123'; + + createComponent({ + actions: { + diffs: { + setCurrentFileHash: currentHash, + fetchFileByFile: fetchFbf, + }, + }, + extendStore: ({ state }) => { + state.diffs.treeEntries = { + 123: { + type: 'blob', + fileHash: '123', + filePaths: { old: '1234', new: '123' }, + parentPath: '/', + }, + 312: { + type: 'blob', + fileHash: '312', + filePaths: { old: '3124', new: '312' }, + parentPath: '/', + }, + }; + state.diffs.diffFiles = [{ file_hash: '123' }, { file_hash: '312' }]; + }, + baseConfig: { viewDiffsFileByFile: true }, + }); + }); + + it.each` + hash | updated | alias + ${'312'} | ${'312'} | ${'312'} + ${''} | ${'123'} | ${'(nothing)'} + `( + 'reacts to the hash changing to "$alias" externally (e.g. browser back/forward)', + async ({ hash, updated }) => { + window.location.hash = hash; + window.dispatchEvent(new Event('hashchange')); + + await nextTick(); + + expect(currentHash).toHaveBeenCalledWith(expect.anything(), updated); + expect(fetchFbf).toHaveBeenCalled(); + }, + ); + }); }); describe('autoscroll', () => { -- GitLab From cd530a3d3197f7a56320d2d6a9c0d0cde57749ce Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 27 Mar 2024 20:27:59 -0600 Subject: [PATCH 3/3] Add test for non-file-by-file mode hash changes --- spec/frontend/diffs/components/app_spec.js | 131 ++++++++++++++------- 1 file changed, 90 insertions(+), 41 deletions(-) diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 745c99636cfb21..69f40e9875d1d2 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -816,58 +816,107 @@ describe('diffs/components/app', () => { }); describe('non-UI navigation', () => { - let currentHash; - let fetchFbf; + describe('in single-file review mode', () => { + let currentHash; + let fetchFbf; - beforeEach(() => { - currentHash = jest.fn(); - fetchFbf = jest.fn(); - window.location.hash = '123'; + beforeEach(() => { + currentHash = jest.fn(); + fetchFbf = jest.fn(); + window.location.hash = '123'; - createComponent({ - actions: { - diffs: { - setCurrentFileHash: currentHash, - fetchFileByFile: fetchFbf, + createComponent({ + actions: { + diffs: { + setCurrentFileHash: currentHash, + fetchFileByFile: fetchFbf, + }, + }, + extendStore: ({ state }) => { + state.diffs.treeEntries = { + 123: { + type: 'blob', + fileHash: '123', + filePaths: { old: '1234', new: '123' }, + parentPath: '/', + }, + 312: { + type: 'blob', + fileHash: '312', + filePaths: { old: '3124', new: '312' }, + parentPath: '/', + }, + }; + state.diffs.diffFiles = [{ file_hash: '123' }, { file_hash: '312' }]; }, + baseConfig: { viewDiffsFileByFile: true }, + }); + }); + + it.each` + hash | updated | alias + ${'312'} | ${'312'} | ${'312'} + ${''} | ${'123'} | ${'(nothing)'} + `( + 'reacts to the hash changing to "$alias" externally (e.g. browser back/forward)', + async ({ hash, updated }) => { + window.location.hash = hash; + window.dispatchEvent(new Event('hashchange')); + + await nextTick(); + + expect(currentHash).toHaveBeenCalledWith(expect.anything(), updated); + expect(fetchFbf).toHaveBeenCalled(); }, - extendStore: ({ state }) => { - state.diffs.treeEntries = { - 123: { - type: 'blob', - fileHash: '123', - filePaths: { old: '1234', new: '123' }, - parentPath: '/', - }, - 312: { - type: 'blob', - fileHash: '312', - filePaths: { old: '3124', new: '312' }, - parentPath: '/', + ); + }); + + describe('in "normal" (multi-file) mode', () => { + let currentHash; + let fetchFbf; + + beforeEach(() => { + currentHash = jest.fn(); + fetchFbf = jest.fn(); + window.location.hash = '123'; + + createComponent({ + actions: { + diffs: { + setCurrentFileHash: currentHash, + fetchFileByFile: fetchFbf, }, - }; - state.diffs.diffFiles = [{ file_hash: '123' }, { file_hash: '312' }]; - }, - baseConfig: { viewDiffsFileByFile: true }, + }, + extendStore: ({ state }) => { + state.diffs.treeEntries = { + 123: { + type: 'blob', + fileHash: '123', + filePaths: { old: '1234', new: '123' }, + parentPath: '/', + }, + 312: { + type: 'blob', + fileHash: '312', + filePaths: { old: '3124', new: '312' }, + parentPath: '/', + }, + }; + state.diffs.diffFiles = [{ file_hash: '123' }, { file_hash: '312' }]; + }, + }); }); - }); - it.each` - hash | updated | alias - ${'312'} | ${'312'} | ${'312'} - ${''} | ${'123'} | ${'(nothing)'} - `( - 'reacts to the hash changing to "$alias" externally (e.g. browser back/forward)', - async ({ hash, updated }) => { - window.location.hash = hash; + it('does not react to the hash changing when in regular (multi-file) mode', async () => { + window.location.hash = '312'; window.dispatchEvent(new Event('hashchange')); await nextTick(); - expect(currentHash).toHaveBeenCalledWith(expect.anything(), updated); - expect(fetchFbf).toHaveBeenCalled(); - }, - ); + expect(currentHash).not.toHaveBeenCalled(); + expect(fetchFbf).not.toHaveBeenCalled(); + }); + }); }); }); -- GitLab