From 0407c25006f8dc867f03087d5b217571bf25b326 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Thu, 4 Dec 2025 17:40:00 -0500 Subject: [PATCH 1/2] Support hiding discussions on commit diffs --- .../rapid_diffs/adapters/discussions.js | 6 + .../rapid_diffs/adapters/options_menu.js | 39 +++--- .../rapid_diffs/app/adapter_configs/commit.js | 14 ++- .../commit_diffs_file_options_dropdown.vue | 80 ++++++++++++ .../diff_file_options_dropdown.vue | 51 ++++++-- .../rapid_diffs/stores/diff_discussions.js | 26 ++++ locale/gitlab.pot | 6 + .../rapid_diffs/adapters/discussions_spec.js | 78 ++++++++++++ .../rapid_diffs/adapters/options_menu_spec.js | 114 ++++++++++++----- .../commit_diffs_file_optons_dropdown_spec.js | 115 ++++++++++++++++++ .../diff_file_options_dropdown_spec.js | 64 +++++++++- .../stores/diff_discussions_spec.js | 89 ++++++++++++++ 12 files changed, 618 insertions(+), 64 deletions(-) create mode 100644 app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue create mode 100644 spec/frontend/rapid_diffs/app/options_menu/commit_diffs_file_optons_dropdown_spec.js diff --git a/app/assets/javascripts/rapid_diffs/adapters/discussions.js b/app/assets/javascripts/rapid_diffs/adapters/discussions.js index fdb28d8dbf6efc..eda1ec7f414c9e 100644 --- a/app/assets/javascripts/rapid_diffs/adapters/discussions.js +++ b/app/assets/javascripts/rapid_diffs/adapters/discussions.js @@ -33,6 +33,8 @@ function mountVueApp(el, id, appData) { render(h) { if (!this.discussion) return null; + if (this.discussion.hidden) return null; + if (this.discussion.isForm) { return h(NewLineDiscussionForm, { props: { discussion: this.discussion } }); } @@ -175,6 +177,8 @@ export const parallelDiscussionsAdapter = { newDiscussion(event, button) { const [oldLine, newLine] = getParallelPosition(button); const { oldPath, newPath } = this.data; + // Show all discussions for this file when adding a new discussion + useDiffDiscussions(pinia).toggleFileDiscussions(oldPath, newPath, false); const existingDiscussionId = useDiffDiscussions(pinia).addNewLineDiscussionForm({ oldPath, newPath, @@ -199,6 +203,8 @@ export const inlineDiscussionsAdapter = { newDiscussion(event, button) { const [oldLine, newLine] = getInlinePosition(button); const { oldPath, newPath } = this.data; + // Show all discussions for this file when adding a new discussion + useDiffDiscussions(pinia).toggleFileDiscussions(oldPath, newPath, false); const existingDiscussionId = useDiffDiscussions(pinia).addNewLineDiscussionForm({ oldPath, newPath, diff --git a/app/assets/javascripts/rapid_diffs/adapters/options_menu.js b/app/assets/javascripts/rapid_diffs/adapters/options_menu.js index f8f336f94f8025..8aa2c7f3870a0f 100644 --- a/app/assets/javascripts/rapid_diffs/adapters/options_menu.js +++ b/app/assets/javascripts/rapid_diffs/adapters/options_menu.js @@ -1,24 +1,33 @@ import Vue from 'vue'; +import { pinia } from '~/pinia/instance'; import DiffFileOptionsDropdown from '~/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue'; +import CommitDiffsFileOptionsDropdown from '~/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue'; function getMenuItems(container) { return JSON.parse(container.querySelector('script').textContent); } -export const optionsMenuAdapter = { - clicks: { - toggleOptionsMenu(event, button) { - const menuContainer = this.diffElement.querySelector('[data-options-menu]'); - if (!menuContainer) return; - const items = getMenuItems(menuContainer); - // eslint-disable-next-line no-new - new Vue({ - el: Vue.version.startsWith('2') ? button : menuContainer, - name: 'GlDisclosureDropdown', - render(h) { - return h(DiffFileOptionsDropdown, { props: { items } }); - }, - }); +const createOptionsMenuAdaptor = (dropdownComponent) => { + return { + clicks: { + toggleOptionsMenu(event, button) { + const menuContainer = this.diffElement.querySelector('[data-options-menu]'); + if (!menuContainer) return; + const items = getMenuItems(menuContainer); + const { oldPath, newPath } = this.data; + // eslint-disable-next-line no-new + new Vue({ + el: Vue.version.startsWith('2') ? button : menuContainer, + name: 'GlDisclosureDropdown', + pinia, + render(h) { + return h(dropdownComponent, { props: { items, oldPath, newPath } }); + }, + }); + }, }, - }, + }; }; + +export const optionsMenuAdapter = createOptionsMenuAdaptor(DiffFileOptionsDropdown); +export const commitOptionsMenuAdaptor = createOptionsMenuAdaptor(CommitDiffsFileOptionsDropdown); diff --git a/app/assets/javascripts/rapid_diffs/app/adapter_configs/commit.js b/app/assets/javascripts/rapid_diffs/app/adapter_configs/commit.js index 594a3a68598cdd..0b5e33934e5aa4 100644 --- a/app/assets/javascripts/rapid_diffs/app/adapter_configs/commit.js +++ b/app/assets/javascripts/rapid_diffs/app/adapter_configs/commit.js @@ -3,9 +3,17 @@ import { inlineDiscussionsAdapter, parallelDiscussionsAdapter, } from '~/rapid_diffs/adapters/discussions'; +import { optionsMenuAdapter, commitOptionsMenuAdaptor } from '~/rapid_diffs/adapters/options_menu'; export const adapters = { - ...VIEWER_ADAPTERS, - text_inline: [...VIEWER_ADAPTERS.text_inline, inlineDiscussionsAdapter], - text_parallel: [...VIEWER_ADAPTERS.text_parallel, parallelDiscussionsAdapter], + text_inline: [ + ...VIEWER_ADAPTERS.text_inline.filter((a) => a !== optionsMenuAdapter), + commitOptionsMenuAdaptor, + inlineDiscussionsAdapter, + ], + text_parallel: [ + ...VIEWER_ADAPTERS.text_parallel.filter((a) => a !== optionsMenuAdapter), + commitOptionsMenuAdaptor, + parallelDiscussionsAdapter, + ], }; diff --git a/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue b/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue new file mode 100644 index 00000000000000..bd9fed53603c9d --- /dev/null +++ b/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue @@ -0,0 +1,80 @@ + + + diff --git a/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue b/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue index 69a6b32dc470f4..ace982a10f21aa 100644 --- a/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue +++ b/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue @@ -1,16 +1,31 @@ @@ -49,7 +53,7 @@ describe('Diff File Options Menu', () => { `; get('file').mount({ - adapterConfig: { [viewer]: [optionsMenuAdapter] }, + adapterConfig: { [viewer]: [adapter] }, appData: {}, unobserve: jest.fn(), }); @@ -59,40 +63,88 @@ describe('Diff File Options Menu', () => { customElements.define('diff-file', DiffFile); }); - beforeEach(() => { - mount(); - }); + describe('optionsMenuAdapter', () => { + beforeEach(() => { + mount(optionsMenuAdapter); + }); - it('starts with the server-rendered button', () => { - expect(get('serverButton')).not.toBeNull(); - }); + it('starts with the server-rendered button', () => { + expect(get('serverButton')).not.toBeNull(); + }); + + it('replaces the server-rendered button with a Vue GlDisclosureDropdown when the button is clicked', () => { + const button = get('serverButton'); + + expect(get('vueButton')).toBeNull(); + expect(button).not.toBeNull(); - it('replaces the server-rendered button with a Vue GlDisclosureDropdown when the button is clicked', () => { - const button = get('serverButton'); + delegatedClick(button); - expect(get('vueButton')).toBeNull(); - expect(button).not.toBeNull(); + expect(get('vueButton')).not.toBeNull(); + /* + * This button being replaced also means this replacement can only + * happen once (desireable!), so testing that it's no longer present is good + */ + expect(get('serverButton')).toBeNull(); + expect(document.activeElement).toEqual(get('vueButton')); + }); + + it('renders the correct menu items in the GlDisclosureDropdown as provided by the back end', () => { + const button = get('serverButton'); + + delegatedClick(button); - delegatedClick(button); + const items = Array.from(get('menuItems')); - expect(get('vueButton')).not.toBeNull(); - /* - * This button being replaced also means this replacement can only - * happen once (desireable!), so testing that it's no longer present is good - */ - expect(get('serverButton')).toBeNull(); - expect(document.activeElement).toEqual(get('vueButton')); + expect(items).toHaveLength(1); + expect(items[0].textContent.trim()).toBe(item1.text); + expect(items[0].querySelector('a').getAttribute('href')).toBe(item1.href); + }); }); - it('renders the correct menu items in the GlDisclosureDropdown as provided by the back end', () => { - const button = get('serverButton'); + describe('commitOptionsMenuAdaptor', () => { + beforeEach(() => { + setActivePinia(pinia); + mount(commitOptionsMenuAdaptor); + }); + + it('starts with the server-rendered button', () => { + expect(get('serverButton')).not.toBeNull(); + }); + + it('replaces the server-rendered button with CommitDiffsFileOptionsDropdown when clicked', () => { + const button = get('serverButton'); + + expect(get('vueButton')).toBeNull(); + expect(button).not.toBeNull(); + + delegatedClick(button); + + expect(get('vueButton')).not.toBeNull(); + expect(get('serverButton')).toBeNull(); + expect(document.activeElement).toEqual(get('vueButton')); + }); - delegatedClick(button); + it('renders the correct menu items in the dropdown as provided by the back end', () => { + const button = get('serverButton'); - const items = Array.from(get('menuItems')); + delegatedClick(button); - expect(items).toHaveLength(1); - expect(items[0].textContent.trim()).toBe(item1.text); - expect(items[0].querySelector('a').getAttribute('href')).toBe(item1.path); + const items = Array.from(get('menuItems')); + + expect(items).toHaveLength(1); + expect(items[0].textContent.trim()).toBe(item1.text); + expect(items[0].querySelector('a').getAttribute('href')).toBe(item1.href); + }); + + it('passes oldPath and newPath props to the dropdown component', () => { + const button = get('serverButton'); + + delegatedClick(button); + + // The component should be mounted with oldPath and newPath props + // This is verified by the component rendering correctly with file-specific features + expect(get('vueButton')).not.toBeNull(); + }); }); }); diff --git a/spec/frontend/rapid_diffs/app/options_menu/commit_diffs_file_optons_dropdown_spec.js b/spec/frontend/rapid_diffs/app/options_menu/commit_diffs_file_optons_dropdown_spec.js new file mode 100644 index 00000000000000..4d99c58ebc3160 --- /dev/null +++ b/spec/frontend/rapid_diffs/app/options_menu/commit_diffs_file_optons_dropdown_spec.js @@ -0,0 +1,115 @@ +import Vue, { nextTick } from 'vue'; +import { createTestingPinia } from '@pinia/testing'; +import { PiniaVuePlugin } from 'pinia'; +import { mountExtended } from 'helpers/vue_test_utils_helper'; +import DiffFileOptionsDropdown from '~/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue'; +import { useDiffDiscussions } from '~/rapid_diffs/stores/diff_discussions'; +import CommitDiffFileOptionsDropdown from '~/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue'; + +Vue.use(PiniaVuePlugin); + +describe('CommitDiffFileOptionsDropdown', () => { + let wrapper; + let pinia; + let store; + + const createComponent = (props = {}) => { + wrapper = mountExtended(CommitDiffFileOptionsDropdown, { + propsData: { + items: [{ text: 'View file', href: '/file' }], + oldPath: 'file.js', + newPath: 'file.js', + ...props, + }, + pinia, + }); + }; + + const findToggleCommentButton = () => wrapper.findByTestId('toggle-comment-button'); + + beforeEach(() => { + pinia = createTestingPinia({ stubActions: false }); + store = useDiffDiscussions(); + }); + + describe('when file has no discussions', () => { + beforeEach(() => { + // store.setInitialDiscussions([]); + createComponent(); + }); + + it('passes groups with only base items', () => { + const dropdown = wrapper.findComponent(DiffFileOptionsDropdown); + expect(dropdown.props('items')).toHaveLength(1); + // expect(groups[0].items).toEqual([{ text: 'View file', href: '/file' }]); + }); + }); + + describe('when file has discussions', () => { + beforeEach(() => { + store.setInitialDiscussions([ + { + id: '1', + diff_discussion: true, + position: { old_path: 'file.js', new_path: 'file.js' }, + notes: [], + hidden: false, + }, + ]); + createComponent(); + }); + + it('includes toggle comments item in a bordered group', () => { + const dropdown = wrapper.findComponent(DiffFileOptionsDropdown); + expect(dropdown.props('items')).toHaveLength(2); + expect(findToggleCommentButton().exists()).toBe(true); + expect(findToggleCommentButton().text()).toBe('Hide comments on this file'); + }); + + it('shows "Hide comments" when discussions are visible', () => { + expect(findToggleCommentButton().text()).toBe('Hide comments on this file'); + }); + + it('shows "Show comments" when discussions are hidden', async () => { + store.discussions[0].hidden = true; + + await nextTick(); + + expect(findToggleCommentButton().text()).toBe('Show comments on this file'); + }); + }); + + describe('toggleComments', () => { + beforeEach(() => { + store.setInitialDiscussions([ + { + id: '1', + diff_discussion: true, + position: { old_path: 'file.js', new_path: 'file.js' }, + notes: [], + hidden: false, + }, + ]); + createComponent(); + }); + + it('calls store action to hide discussions when currently visible', () => { + jest.spyOn(store, 'toggleFileDiscussions'); + + findToggleCommentButton().trigger('click'); + + expect(store.toggleFileDiscussions).toHaveBeenCalledWith('file.js', 'file.js', true); + }); + + it('calls store action to show discussions when currently hidden', async () => { + store.discussions[0].hidden = true; + await nextTick(); + + jest.spyOn(store, 'toggleFileDiscussions'); + + findToggleCommentButton().trigger('click'); + + expect(store.toggleFileDiscussions).toHaveBeenCalledWith('file.js', 'file.js', false); + }); + }); +}); diff --git a/spec/frontend/rapid_diffs/app/options_menu/diff_file_options_dropdown_spec.js b/spec/frontend/rapid_diffs/app/options_menu/diff_file_options_dropdown_spec.js index 7beae96199a024..e9cfe13924d1bd 100644 --- a/spec/frontend/rapid_diffs/app/options_menu/diff_file_options_dropdown_spec.js +++ b/spec/frontend/rapid_diffs/app/options_menu/diff_file_options_dropdown_spec.js @@ -1,4 +1,5 @@ import { mount } from '@vue/test-utils'; +import { GlDisclosureDropdownGroup, GlDisclosureDropdownItem } from '@gitlab/ui'; import DiffFileOptionsDropdown from '~/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue'; describe('DiffFileOptionsDropdown', () => { @@ -9,10 +10,65 @@ describe('DiffFileOptionsDropdown', () => { wrapper = mount(DiffFileOptionsDropdown, { propsData }); }; - it('shows options', () => { - const item = { text: 'View file' }; - createComponent({ items: [item] }); - expect(wrapper.html()).toContain(item.text); + const findDropdownGroups = () => wrapper.findAllComponents(GlDisclosureDropdownGroup); + const findDropdownItems = () => wrapper.findAllComponents(GlDisclosureDropdownItem); + + describe('with flat items', () => { + beforeEach(() => { + createComponent({ items: [{ text: 'View file' }, { text: 'Download' }] }); + }); + + it('renders items without groups', () => { + expect(findDropdownGroups()).toHaveLength(0); + expect(findDropdownItems()).toHaveLength(2); + }); + + it('shows options', () => { + expect(wrapper.html()).toContain('View file'); + expect(wrapper.html()).toContain('Download'); + }); + }); + + describe('with grouped items', () => { + beforeEach(() => { + createComponent({ + items: [ + { + name: 'File actions', + items: [{ text: 'View file' }, { text: 'Download' }], + }, + { + name: 'Comments', + bordered: true, + items: [{ text: 'Hide comments' }], + }, + ], + }); + }); + + it('renders groups', () => { + expect(findDropdownGroups()).toHaveLength(2); + expect(findDropdownItems()).toHaveLength(3); + }); + + it('shows all group items', () => { + expect(wrapper.html()).toContain('View file'); + expect(wrapper.html()).toContain('Download'); + expect(wrapper.html()).toContain('Hide comments'); + }); + + it('passes group properties to GlDisclosureDropdownGroup', () => { + const groups = findDropdownGroups(); + expect(groups.at(0).props('group')).toMatchObject({ + name: 'File actions', + items: expect.any(Array), + }); + expect(groups.at(1).props('group')).toMatchObject({ + name: 'Comments', + bordered: true, + items: expect.any(Array), + }); + }); }); it('renders code tags', () => { diff --git a/spec/frontend/rapid_diffs/stores/diff_discussions_spec.js b/spec/frontend/rapid_diffs/stores/diff_discussions_spec.js index 312ef3d510474b..aa7dfdf6bfb43f 100644 --- a/spec/frontend/rapid_diffs/stores/diff_discussions_spec.js +++ b/spec/frontend/rapid_diffs/stores/diff_discussions_spec.js @@ -14,6 +14,7 @@ describe('diffDiscussions store', () => { expect(useDiffDiscussions().discussions[0].repliesExpanded).toBe(true); expect(useDiffDiscussions().discussions[0].notes[0].isEditing).toBe(false); expect(useDiffDiscussions().discussions[0].notes[0].editedNote).toBeNull(); + expect(useDiffDiscussions().discussions[0].hidden).toBe(false); }); }); @@ -398,6 +399,43 @@ describe('diffDiscussions store', () => { }); }); + describe('toggleFileDiscussions', () => { + beforeEach(() => { + useDiffDiscussions().discussions = [ + { + id: '1', + diff_discussion: true, + position: { old_path: 'file1.js', new_path: 'file1.js' }, + }, + { + id: '2', + diff_discussion: true, + position: { old_path: 'file1.js', new_path: 'file1.js' }, + }, + { + id: '3', + diff_discussion: true, + position: { old_path: 'file2.js', new_path: 'file2.js' }, + }, + ]; + }); + + it('hides all discussions for a file when newState is true', () => { + useDiffDiscussions().toggleFileDiscussions('file1.js', 'file1.js', true); + + expect(useDiffDiscussions().discussions[0].hidden).toBe(true); + expect(useDiffDiscussions().discussions[1].hidden).toBe(true); + }); + + it('shows all discussions for a file when newState is false', () => { + useDiffDiscussions().toggleFileDiscussions('file1.js', 'file1.js', true); + useDiffDiscussions().toggleFileDiscussions('file1.js', 'file1.js', false); + + expect(useDiffDiscussions().discussions[0].hidden).toBe(false); + expect(useDiffDiscussions().discussions[1].hidden).toBe(false); + }); + }); + describe('allNotesById', () => { it('returns all notes by id', () => { const note1 = { id: 'foo' }; @@ -467,4 +505,55 @@ describe('diffDiscussions store', () => { expect(found).toHaveLength(0); }); }); + + describe('findDiscussionsForFile', () => { + beforeEach(() => { + useDiffDiscussions().discussions = [ + { + id: '1', + diff_discussion: true, + position: { old_path: 'file1.js', new_path: 'file1.js' }, + }, + { + id: '2', + diff_discussion: true, + position: { old_path: 'file2.js', new_path: 'file2.js' }, + }, + { + id: '3', + isForm: true, + diff_discussion: true, + position: { old_path: 'file1.js', new_path: 'file1.js' }, + }, + ]; + }); + + it('returns discussions matching the file paths', () => { + const discussions = useDiffDiscussions().findDiscussionsForFile({ + oldPath: 'file1.js', + newPath: 'file1.js', + }); + + expect(discussions).toHaveLength(1); + expect(discussions[0].id).toBe('1'); + }); + + it('excludes discussion forms', () => { + const discussions = useDiffDiscussions().findDiscussionsForFile({ + oldPath: 'file1.js', + newPath: 'file1.js', + }); + + expect(discussions.every((d) => !d.isForm)).toBe(true); + }); + + it('returns empty array when no discussions match', () => { + const discussions = useDiffDiscussions().findDiscussionsForFile({ + oldPath: 'nonexistent.js', + newPath: 'nonexistent.js', + }); + + expect(discussions).toHaveLength(0); + }); + }); }); -- GitLab From 5734ecba0d4fba407af090f788d06549bb0c9320 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Wed, 10 Dec 2025 16:40:09 -0500 Subject: [PATCH 2/2] Close dropdown when button is clicked --- .../commit_diffs_file_options_dropdown.vue | 3 ++- .../options_menu/diff_file_options_dropdown.vue | 5 +++++ ... => commit_diffs_file_options_dropdown_spec.js} | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-) rename spec/frontend/rapid_diffs/app/options_menu/{commit_diffs_file_optons_dropdown_spec.js => commit_diffs_file_options_dropdown_spec.js} (89%) diff --git a/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue b/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue index bd9fed53603c9d..d479529f5c1f53 100644 --- a/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue +++ b/app/assets/javascripts/rapid_diffs/app/options_menu/commit_diffs_file_options_dropdown.vue @@ -70,11 +70,12 @@ export default { ...mapActions(useDiffDiscussions, ['toggleFileDiscussions']), toggleComments() { this.toggleFileDiscussions(this.oldPath, this.newPath, !this.discussionsHidden); + this.$refs['diff-file-options-dropdown']?.closeAndFocus(); }, }, }; diff --git a/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue b/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue index ace982a10f21aa..376b07e3603b19 100644 --- a/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue +++ b/app/assets/javascripts/rapid_diffs/app/options_menu/diff_file_options_dropdown.vue @@ -45,12 +45,17 @@ export default { if (!item.messageData) return item.text; return sprintf(item.text, item.messageData); }, + // eslint-disable-next-line vue/no-unused-properties + closeAndFocus() { + this.$refs.dropdown.closeAndFocus(); + }, }, };