From cc105d24e1ebd4f3fbd66599c44d119fc52de417 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Thu, 6 Mar 2025 18:35:54 -0500 Subject: [PATCH 1/3] Fix blob overflow menu permalink shortcut does not get triggered This happens when user navigate to the blob page from repository page. The `Mousetrap.bind()` in mounted() is getting ovewritten by the shortcuts_blob.js, which we use to trigger the old permalink button (when flag is off). This adds a feature flag check in `shortcuts_blob.js. Additionally, this also adds a new const under keybindings.js that better reflects the nature of the new permalink (e.g. copy). This feature was originally added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/181901 and is behind the `blob_overflow_menu` feature flag. --- .../behaviors/shortcuts/keybindings.js | 6 ++ .../behaviors/shortcuts/shortcuts_blob.js | 6 ++ .../header_area/permalink_dropdown_item.vue | 8 +-- .../shortcuts/shortcuts_blob_spec.js | 69 +++++++++++++++++++ 4 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js diff --git a/app/assets/javascripts/behaviors/shortcuts/keybindings.js b/app/assets/javascripts/behaviors/shortcuts/keybindings.js index 10c22609858cbd..aec48e4b20eafe 100644 --- a/app/assets/javascripts/behaviors/shortcuts/keybindings.js +++ b/app/assets/javascripts/behaviors/shortcuts/keybindings.js @@ -394,6 +394,12 @@ export const PROJECT_FILES_GO_TO_PERMALINK = { defaultKeys: ['y'], }; +export const PROJECT_FILES_COPY_PERMALINK = { + id: 'projectFiles.copyPermalink', + description: __('Go to permalink'), + defaultKeys: ['y'], +}; + export const PROJECT_FILES_GO_TO_COMPARE = { id: 'projectFiles.goToCompare', description: __('Compare Branches'), diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index 9f8d7272e5c52e..707928f9e9e749 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -3,6 +3,12 @@ import { moveToFilePermalink } from '~/blob/utils'; export default class ShortcutsBlob { constructor(shortcuts) { + const { blobOverflowMenu, blobRepositoryVueHeaderApp } = gon.features ?? {}; + if (blobOverflowMenu && blobRepositoryVueHeaderApp) { + // TODO: Remove ShortcutsBlob entirely once these feature flags are removed. + return; + } + shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, moveToFilePermalink); } } diff --git a/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue b/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue index 20804f914c1eda..bd954874d4182b 100644 --- a/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue +++ b/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue @@ -2,7 +2,7 @@ import Vue from 'vue'; import { GlDisclosureDropdownGroup, GlDisclosureDropdownItem, GlToast } from '@gitlab/ui'; import { __ } from '~/locale'; -import { keysFor, PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; +import { keysFor, PROJECT_FILES_COPY_PERMALINK } from '~/behaviors/shortcuts/keybindings'; import { Mousetrap } from '~/lib/mousetrap'; import { shouldDisableShortcuts } from '~/behaviors/shortcuts/shortcuts_toggle'; import { getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; @@ -24,7 +24,7 @@ export default { }, computed: { permalinkShortcutKey() { - return keysFor(PROJECT_FILES_GO_TO_PERMALINK)[0]; + return keysFor(PROJECT_FILES_COPY_PERMALINK)[0]; }, shortcutsDisabled() { return shouldDisableShortcuts(); @@ -40,10 +40,10 @@ export default { }, }, mounted() { - Mousetrap.bind(keysFor(PROJECT_FILES_GO_TO_PERMALINK), this.triggerCopyPermalink); + Mousetrap.bind(keysFor(PROJECT_FILES_COPY_PERMALINK), this.triggerCopyPermalink); }, beforeDestroy() { - Mousetrap.unbind(keysFor(PROJECT_FILES_GO_TO_PERMALINK)); + Mousetrap.unbind(keysFor(PROJECT_FILES_COPY_PERMALINK)); }, methods: { triggerCopyPermalink() { diff --git a/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js new file mode 100644 index 00000000000000..cd67a031ef78f7 --- /dev/null +++ b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js @@ -0,0 +1,69 @@ +import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; +import { PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; +import { moveToFilePermalink } from '~/blob/utils'; + +jest.mock('~/blob/utils', () => ({ + moveToFilePermalink: jest.fn(), +})); + +describe('ShortcutsBlob', () => { + let shortcuts; + + const init = () => { + return new ShortcutsBlob(shortcuts); + }; + + beforeEach(() => { + shortcuts = { + add: jest.fn(), + }; + + window.gon = { features: {} }; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('constructor', () => { + it('adds the permalink shortcut when gon.features is undefined', () => { + window.gon = {}; + + init(); + + expect(shortcuts.add).toHaveBeenCalledWith( + PROJECT_FILES_GO_TO_PERMALINK, + moveToFilePermalink, + ); + }); + + describe.each` + blobOverflowMenu | blobRepositoryVueHeaderApp | shouldAddShortcuts + ${false} | ${false} | ${true} + ${true} | ${false} | ${true} + ${false} | ${true} | ${true} + ${true} | ${true} | ${false} + `( + 'feature flag combinations', + ({ blobOverflowMenu, blobRepositoryVueHeaderApp, shouldAddShortcuts }) => { + it(`${shouldAddShortcuts ? 'adds' : 'does not add'} shortcuts when blobOverflowMenu is ${blobOverflowMenu} and blobRepositoryVueHeaderApp is ${blobRepositoryVueHeaderApp}`, () => { + window.gon.features = { + blobOverflowMenu, + blobRepositoryVueHeaderApp, + }; + + init(); + + if (shouldAddShortcuts) { + expect(shortcuts.add).toHaveBeenCalledWith( + PROJECT_FILES_GO_TO_PERMALINK, + moveToFilePermalink, + ); + } else { + expect(shortcuts.add).not.toHaveBeenCalled(); + } + }); + }, + ); + }); +}); -- GitLab From cd9ede411ef2a8e9292b4413e2fc2a7d8bef3e63 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Mon, 10 Mar 2025 15:32:59 -0400 Subject: [PATCH 2/3] Address review comments in tests --- .../behaviors/shortcuts/keybindings.js | 6 +- locale/gitlab.pot | 3 + .../shortcuts/shortcuts_blob_spec.js | 69 ++++++++----------- 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/keybindings.js b/app/assets/javascripts/behaviors/shortcuts/keybindings.js index aec48e4b20eafe..2112ba5c9d816b 100644 --- a/app/assets/javascripts/behaviors/shortcuts/keybindings.js +++ b/app/assets/javascripts/behaviors/shortcuts/keybindings.js @@ -388,9 +388,13 @@ export const PROJECT_FILES_GO_BACK = { defaultKeys: ['esc'], }; +const { blobOverflowMenu, blobRepositoryVueHeaderApp } = gon.features ?? {}; export const PROJECT_FILES_GO_TO_PERMALINK = { id: 'projectFiles.goToFilePermalink', - description: __('Go to file permalink (while viewing a file)'), + description: + blobOverflowMenu && blobRepositoryVueHeaderApp + ? __('Copy file permalink') + : __('Go to file permalink (while viewing a file)'), defaultKeys: ['y'], }; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6e98507be8f398..335ab7bcc4e1ae 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16860,6 +16860,9 @@ msgstr "" msgid "Copy file path" msgstr "" +msgid "Copy file permalink" +msgstr "" + msgid "Copy image URL" msgstr "" diff --git a/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js index cd67a031ef78f7..ece0582ef92c53 100644 --- a/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js +++ b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js @@ -2,33 +2,17 @@ import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; import { PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; import { moveToFilePermalink } from '~/blob/utils'; -jest.mock('~/blob/utils', () => ({ - moveToFilePermalink: jest.fn(), -})); - describe('ShortcutsBlob', () => { - let shortcuts; + const shortcuts = { + add: jest.fn(), + }; const init = () => { return new ShortcutsBlob(shortcuts); }; - beforeEach(() => { - shortcuts = { - add: jest.fn(), - }; - - window.gon = { features: {} }; - }); - - afterEach(() => { - jest.clearAllMocks(); - }); - describe('constructor', () => { it('adds the permalink shortcut when gon.features is undefined', () => { - window.gon = {}; - init(); expect(shortcuts.add).toHaveBeenCalledWith( @@ -42,28 +26,33 @@ describe('ShortcutsBlob', () => { ${false} | ${false} | ${true} ${true} | ${false} | ${true} ${false} | ${true} | ${true} - ${true} | ${true} | ${false} - `( - 'feature flag combinations', - ({ blobOverflowMenu, blobRepositoryVueHeaderApp, shouldAddShortcuts }) => { - it(`${shouldAddShortcuts ? 'adds' : 'does not add'} shortcuts when blobOverflowMenu is ${blobOverflowMenu} and blobRepositoryVueHeaderApp is ${blobRepositoryVueHeaderApp}`, () => { - window.gon.features = { - blobOverflowMenu, - blobRepositoryVueHeaderApp, - }; + `('when shortcuts should be added', ({ blobOverflowMenu, blobRepositoryVueHeaderApp }) => { + it(`adds shortcuts when blobOverflowMenu is ${blobOverflowMenu} and blobRepositoryVueHeaderApp is ${blobRepositoryVueHeaderApp}`, () => { + window.gon.features = { + blobOverflowMenu, + blobRepositoryVueHeaderApp, + }; + + init(); + + expect(shortcuts.add).toHaveBeenCalledWith( + PROJECT_FILES_GO_TO_PERMALINK, + moveToFilePermalink, + ); + }); + }); - init(); + describe('when shortcuts should not be added', () => { + it('does not add shortcuts when blobOverflowMenu is true and blobRepositoryVueHeaderApp is true', () => { + window.gon.features = { + blobOverflowMenu: true, + blobRepositoryVueHeaderApp: true, + }; - if (shouldAddShortcuts) { - expect(shortcuts.add).toHaveBeenCalledWith( - PROJECT_FILES_GO_TO_PERMALINK, - moveToFilePermalink, - ); - } else { - expect(shortcuts.add).not.toHaveBeenCalled(); - } - }); - }, - ); + init(); + + expect(shortcuts.add).not.toHaveBeenCalled(); + }); + }); }); }); -- GitLab From 15f640a6af262eb7d4c938b37bb763450eeca35a Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Tue, 11 Mar 2025 13:14:08 -0400 Subject: [PATCH 3/3] Remove the new keybinding and header app feature flag check --- .../behaviors/shortcuts/keybindings.js | 15 +++----- .../behaviors/shortcuts/shortcuts_blob.js | 4 +-- .../header_area/permalink_dropdown_item.vue | 8 ++--- .../shortcuts/shortcuts_blob_spec.js | 34 +++++++++---------- 4 files changed, 26 insertions(+), 35 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/keybindings.js b/app/assets/javascripts/behaviors/shortcuts/keybindings.js index 2112ba5c9d816b..c7ee7699d9ca58 100644 --- a/app/assets/javascripts/behaviors/shortcuts/keybindings.js +++ b/app/assets/javascripts/behaviors/shortcuts/keybindings.js @@ -388,19 +388,12 @@ export const PROJECT_FILES_GO_BACK = { defaultKeys: ['esc'], }; -const { blobOverflowMenu, blobRepositoryVueHeaderApp } = gon.features ?? {}; +const { blobOverflowMenu } = gon.features ?? {}; export const PROJECT_FILES_GO_TO_PERMALINK = { id: 'projectFiles.goToFilePermalink', - description: - blobOverflowMenu && blobRepositoryVueHeaderApp - ? __('Copy file permalink') - : __('Go to file permalink (while viewing a file)'), - defaultKeys: ['y'], -}; - -export const PROJECT_FILES_COPY_PERMALINK = { - id: 'projectFiles.copyPermalink', - description: __('Go to permalink'), + description: blobOverflowMenu + ? __('Copy file permalink') + : __('Go to file permalink (while viewing a file)'), defaultKeys: ['y'], }; diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index 707928f9e9e749..13ecfaf1a5493d 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -3,8 +3,8 @@ import { moveToFilePermalink } from '~/blob/utils'; export default class ShortcutsBlob { constructor(shortcuts) { - const { blobOverflowMenu, blobRepositoryVueHeaderApp } = gon.features ?? {}; - if (blobOverflowMenu && blobRepositoryVueHeaderApp) { + const { blobOverflowMenu } = gon.features ?? {}; + if (blobOverflowMenu) { // TODO: Remove ShortcutsBlob entirely once these feature flags are removed. return; } diff --git a/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue b/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue index bd954874d4182b..20804f914c1eda 100644 --- a/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue +++ b/app/assets/javascripts/repository/components/header_area/permalink_dropdown_item.vue @@ -2,7 +2,7 @@ import Vue from 'vue'; import { GlDisclosureDropdownGroup, GlDisclosureDropdownItem, GlToast } from '@gitlab/ui'; import { __ } from '~/locale'; -import { keysFor, PROJECT_FILES_COPY_PERMALINK } from '~/behaviors/shortcuts/keybindings'; +import { keysFor, PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; import { Mousetrap } from '~/lib/mousetrap'; import { shouldDisableShortcuts } from '~/behaviors/shortcuts/shortcuts_toggle'; import { getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; @@ -24,7 +24,7 @@ export default { }, computed: { permalinkShortcutKey() { - return keysFor(PROJECT_FILES_COPY_PERMALINK)[0]; + return keysFor(PROJECT_FILES_GO_TO_PERMALINK)[0]; }, shortcutsDisabled() { return shouldDisableShortcuts(); @@ -40,10 +40,10 @@ export default { }, }, mounted() { - Mousetrap.bind(keysFor(PROJECT_FILES_COPY_PERMALINK), this.triggerCopyPermalink); + Mousetrap.bind(keysFor(PROJECT_FILES_GO_TO_PERMALINK), this.triggerCopyPermalink); }, beforeDestroy() { - Mousetrap.unbind(keysFor(PROJECT_FILES_COPY_PERMALINK)); + Mousetrap.unbind(keysFor(PROJECT_FILES_GO_TO_PERMALINK)); }, methods: { triggerCopyPermalink() { diff --git a/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js index ece0582ef92c53..3bb992636ee075 100644 --- a/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js +++ b/spec/frontend/behaviors/shortcuts/shortcuts_blob_spec.js @@ -11,26 +11,25 @@ describe('ShortcutsBlob', () => { return new ShortcutsBlob(shortcuts); }; + beforeEach(() => { + shortcuts.add.mockClear(); + window.gon = {}; + }); + describe('constructor', () => { - it('adds the permalink shortcut when gon.features is undefined', () => { - init(); + describe('when shortcuts should be added', () => { + it('adds the permalink shortcut when gon.features is undefined', () => { + init(); - expect(shortcuts.add).toHaveBeenCalledWith( - PROJECT_FILES_GO_TO_PERMALINK, - moveToFilePermalink, - ); - }); + expect(shortcuts.add).toHaveBeenCalledWith( + PROJECT_FILES_GO_TO_PERMALINK, + moveToFilePermalink, + ); + }); - describe.each` - blobOverflowMenu | blobRepositoryVueHeaderApp | shouldAddShortcuts - ${false} | ${false} | ${true} - ${true} | ${false} | ${true} - ${false} | ${true} | ${true} - `('when shortcuts should be added', ({ blobOverflowMenu, blobRepositoryVueHeaderApp }) => { - it(`adds shortcuts when blobOverflowMenu is ${blobOverflowMenu} and blobRepositoryVueHeaderApp is ${blobRepositoryVueHeaderApp}`, () => { + it('adds shortcuts when blobOverflowMenu is false', () => { window.gon.features = { - blobOverflowMenu, - blobRepositoryVueHeaderApp, + blobOverflowMenu: false, }; init(); @@ -43,10 +42,9 @@ describe('ShortcutsBlob', () => { }); describe('when shortcuts should not be added', () => { - it('does not add shortcuts when blobOverflowMenu is true and blobRepositoryVueHeaderApp is true', () => { + it('does not add shortcuts when blobOverflowMenu is true', () => { window.gon.features = { blobOverflowMenu: true, - blobRepositoryVueHeaderApp: true, }; init(); -- GitLab