From 81b9de09bed6af133c9a84977ca183e5052c636b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 01:14:19 -0600 Subject: [PATCH 01/15] Auto-collapse a discussion if all threads are resolved --- .../notes/store/legacy_notes/actions.js | 76 +++++++++++++++++++ .../notes/utils/discussion_collapse.js | 67 ++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 app/assets/javascripts/notes/utils/discussion_collapse.js diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 8d6814cc38e7e9..9daad449577631 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -23,6 +23,10 @@ import { TYPENAME_NOTE } from '~/graphql_shared/constants'; import { useBatchComments } from '~/batch_comments/store'; import { uuids } from '~/lib/utils/uuids'; import notesEventHub from '../../event_hub'; +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '../../utils/discussion_collapse'; import promoteTimelineEvent from '../../graphql/promote_timeline_event.mutation.graphql'; @@ -422,9 +426,81 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this.updateResolvableDiscussionsCounts(); this.updateMergeRequestWidget(); + + const discussionId = extractDiscussionIdFromEndpoint(endpoint); + if (discussionId && !isResolved && discussion) { + const collapseAction = determineCollapseAction(this.discussions, discussionId); + if (collapseAction) { + const context = this.getContext(); + this.handleAutoCollapse(collapseAction, { context }); + } + } }); } +export function handleAutoCollapse(collapseAction, options = {}) { + const { context } = options; + + const actionHandlers = { + line: (action) => { + if (context === 'changes') { + this.tryStore('legacyDiffs').toggleLineDiscussions({ + lineCode: action.lineCode, + fileHash: action.fileHash, + expanded: false, + }); + } else { + this.collapseLineDiscussions(action.lineCode); + } + }, + + file: (action) => { + if (context === 'changes') { + action.discussions.forEach((discussion) => { + this.tryStore('legacyDiffs').toggleFileDiscussion({ + discussion, + expandedOnDiff: false, + }); + }); + } else { + action.discussions.forEach((discussion) => { + this.toggleDiscussion({ + discussionId: discussion.id, + forceExpanded: false, + }); + }); + } + }, + + general: (action) => { + this.toggleDiscussion({ + discussionId: action.discussionId, + forceExpanded: false, + }); + }, + }; + + const handler = actionHandlers[collapseAction.type] || (() => {}); + handler(collapseAction); +} + +export function collapseLineDiscussions(lineCode) { + const lineDiscussions = this.discussions.filter((d) => d.line_code === lineCode); + lineDiscussions.forEach((discussion) => { + this.toggleDiscussion({ + discussionId: discussion.id, + forceExpanded: false, + }); + }); +} + +export function getContext() { + const diffStore = this.tryStore('legacyDiffs'); + const hasLoadedDiffs = diffStore && diffStore.diffFiles && diffStore.diffFiles.length > 0; + + return hasLoadedDiffs ? 'changes' : 'overview'; +} + export function closeIssuable() { this.toggleStateButtonLoading(true); return axios.put(this.notesData.closePath).then(({ data }) => { diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js new file mode 100644 index 00000000000000..a1ba3d11cbb0b9 --- /dev/null +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -0,0 +1,67 @@ +/** + * Extracts discussion ID from a resolve endpoint URL + * @param {string} endpoint - The resolve endpoint URL (e.g., "/discussions/abc123/resolve") + * @returns {string|null} - The discussion ID or null if not found + */ +export function extractDiscussionIdFromEndpoint(endpoint) { + if (!endpoint) { + return null; + } + + const match = endpoint.match(/discussions\/([^/]+)\/resolve$/); + return match ? match[1] : null; +} + +/** + * Determines what collapse action should be taken for a resolved discussion + * @param {Array} discussions - Array of all discussions + * @param {string} resolvedDiscussionId - The discussion that was just resolved + * @returns {Object|null} - Single collapse action or null + */ +export function determineCollapseAction(discussions, resolvedDiscussionId) { + const resolvedDiscussion = discussions.find((d) => d.id === resolvedDiscussionId); + let collapseAction = null; + + const isValidDiscussion = resolvedDiscussion && resolvedDiscussion.resolved; + const hasLineCode = isValidDiscussion && Boolean(resolvedDiscussion.line_code); + const hasFileHash = isValidDiscussion && Boolean(resolvedDiscussion.diff_file?.file_hash); + const isGeneralDiscussion = isValidDiscussion && !hasLineCode && !hasFileHash; + + if (hasLineCode) { + const lineDiscussions = discussions.filter((d) => d.line_code === resolvedDiscussion.line_code); + const allLineDiscussionsResolved = lineDiscussions.every((d) => d.resolved); + + collapseAction = allLineDiscussionsResolved + ? { + type: 'line', + lineCode: resolvedDiscussion.line_code, + fileHash: resolvedDiscussion.diff_file?.file_hash, + } + : null; + } + + if (!hasLineCode && hasFileHash) { + const fileHash = resolvedDiscussion.diff_file.file_hash; + const fileDiscussions = discussions.filter( + (d) => d.diff_file?.file_hash === fileHash && !d.line_code, + ); + const allFileDiscussionsResolved = fileDiscussions.every((d) => d.resolved); + + collapseAction = allFileDiscussionsResolved + ? { + type: 'file', + discussions: fileDiscussions, + fileHash, + } + : null; + } + + if (isGeneralDiscussion) { + collapseAction = { + type: 'general', + discussionId: resolvedDiscussion.id, + }; + } + + return collapseAction; +} -- GitLab From ebddca824b78cf71b15c3a2967e1728bb7993a04 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Jul 2025 20:38:14 -0600 Subject: [PATCH 02/15] Expand collapsed discussions before checking for applied status Fixed failing feature specs Attempted fix at QA specs (cherry picked from commit 1c09e2da8d85fe5606eed8544ab05c83c67788e0) Co-authored-by: Phil Hughes Apply 1 suggestion(s) to 1 file(s) (cherry picked from commit d5119ecbe669b7cc71418fa5a758a6a1772c673d) Co-authored-by: Jay McCure Apply 1 suggestion(s) to 1 file(s) (cherry picked from commit db96f84f5a98ce8043879b7dfa67830a9c2697e8) Co-authored-by: Jay McCure --- qa/qa/page/merge_request/show.rb | 4 ++++ .../suggestions/batch_suggestion_spec.rb | 2 ++ .../custom_commit_suggestion_spec.rb | 2 ++ .../user_suggests_changes_on_diff_spec.rb | 18 ++++++++++++++++++ .../helpers/merge_request_diff_helpers.rb | 8 ++++++++ 5 files changed, 34 insertions(+) diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index efa1e2dd1132a9..d39ca2dc7273cc 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -551,6 +551,10 @@ def has_exposed_artifact_with_name?(name) has_link?(name) end + def expand_collapsed_discussions + all_elements('left-discussions', minimum: 1).each(&:click) + end + private def submit_commit diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb index 2b86a7863e8c50..ccd7f4ba6301f1 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb @@ -42,6 +42,8 @@ module QA 4.times { merge_request.add_suggestion_to_batch } merge_request.apply_suggestion_with_message("Custom commit message") + merge_request.expand_collapsed_discussions + expect(merge_request).to have_suggestions_applied(4) end end diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb index 7323a4bb3de45f..ff7a9b9b98273f 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb @@ -38,6 +38,8 @@ module QA merge_request.click_diffs_tab merge_request.apply_suggestion_with_message(commit_message) + merge_request.expand_collapsed_discussions + expect(merge_request).to have_suggestions_applied merge_request.click_commits_tab diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index dd9dc31c2d4f6a..9a2ec37d5b3b37 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -105,7 +105,11 @@ def expect_suggestion_has_content(element, expected_changing_content, expected_s click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_collapsed_discussions + page.within('.diff-discussions') do expect(page).to have_content('Applied') end end @@ -210,6 +214,8 @@ def hash(path) wait_for_requests end + expand_all_collapsed_discussions + expect(page).to have_content('Applied').twice end end @@ -272,7 +278,11 @@ def hash(path) all('button', text: 'Apply suggestion').first.click click_button('Apply') wait_for_requests + end + + expand_collapsed_discussions + page.within(container) do expect(page).to have_content('Applied').once expect(page).to have_button('Apply suggestion').once @@ -392,7 +402,11 @@ def hash(path) click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_collapsed_discussions + page.within("[id='#{hash}']") do expect(page).to have_content('Applied') end end @@ -404,7 +418,11 @@ def hash(path) click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_collapsed_discussions + page.within("[id='#{hash}']") do expect(page).to have_content('Reopen thread') end end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index bbd9382fcc2def..dd296bba31bda8 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -93,4 +93,12 @@ def find_by_scrolling(selector, **options) end element end + + def expand_collapsed_discussions + first('.js-diff-comment-avatar').click + end + + def expand_all_collapsed_discussions + all('.js-diff-comment-avatar').each(&:click) + end end -- GitLab From 97fa5c65bb0dd602bfa8a0c82d61174b7f6d222a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 10:43:59 -0600 Subject: [PATCH 03/15] Test the new collapse utils --- .../notes/utils/discussion_collapse_spec.js | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 spec/frontend/notes/utils/discussion_collapse_spec.js diff --git a/spec/frontend/notes/utils/discussion_collapse_spec.js b/spec/frontend/notes/utils/discussion_collapse_spec.js new file mode 100644 index 00000000000000..5827b3dd2a8345 --- /dev/null +++ b/spec/frontend/notes/utils/discussion_collapse_spec.js @@ -0,0 +1,211 @@ +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '~/notes/utils/discussion_collapse'; + +function createDiscussion(properties = {}) { + return { + id: 'discussion1', + resolved: true, + line_code: null, + diff_file: null, + ...properties, + }; +} + +function createLineDiscussion({ id, resolved = true, lineCode = 'abc_1_1', fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: lineCode, + diff_file: { file_hash: fileHash }, + }); +} + +function createFileDiscussion({ id, resolved = true, fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: { file_hash: fileHash }, + }); +} + +function createGeneralDiscussion({ id, resolved = true }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: null, + }); +} + +describe('Discussion Collapse Utilities', () => { + describe('extractDiscussionIdFromEndpoint', () => { + it.each` + endpoint | expectedResult | description + ${'/discussions/abc123/resolve'} | ${'abc123'} | ${'returns discussion ID from valid endpoint'} + ${'/discussions/abc-123_xyz/resolve'} | ${'abc-123_xyz'} | ${'returns discussion ID with special characters'} + ${'/namespace/project/-/merge_requests/1/discussions/def456/resolve'} | ${'def456'} | ${'returns discussion ID from endpoint with leading path'} + ${'/discussions/abc123/unresolve'} | ${null} | ${'returns null for invalid endpoint format'} + ${'/discussions/abc123'} | ${null} | ${'returns null for endpoint without resolve suffix'} + ${null} | ${null} | ${'returns null for null input'} + ${undefined} | ${null} | ${'returns null for undefined input'} + ${''} | ${null} | ${'returns null for empty string'} + ${'/discussions//resolve'} | ${null} | ${'returns null for malformed endpoint'} + ${'/discussions/abc123/resolve/extra'} | ${null} | ${'returns null for endpoint with extra path segments'} + ${'/discussions/abc123/resolve?param=value'} | ${null} | ${'returns null for endpoint with query parameters'} + `('$description', ({ endpoint, expectedResult }) => { + expect(extractDiscussionIdFromEndpoint(endpoint)).toBe(expectedResult); + }); + }); + + describe('determineCollapseAction', () => { + describe('line discussions', () => { + it('returns line action when all discussions on a line are resolved', () => { + const discussions = [ + createLineDiscussion({ id: 'discussion1' }), + createLineDiscussion({ id: 'discussion2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }); + }); + + it('filters discussions by line_code and ignores other lines', () => { + const discussions = [ + createLineDiscussion({ id: 'discussion1', resolved: true, lineCode: 'abc_1_1' }), + createLineDiscussion({ id: 'discussion2', resolved: true, lineCode: 'abc_2_2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }); + }); + }); + + describe('file-level discussions', () => { + it('returns file action when all file discussions are resolved', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1' }), + createFileDiscussion({ id: 'discussion2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0], discussions[1]], + fileHash: 'file123', + }); + }); + + it('filters file discussions by file_hash and excludes line discussions', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1', resolved: true, fileHash: 'file123' }), + createFileDiscussion({ id: 'discussion2', resolved: true, fileHash: 'file456' }), + createLineDiscussion({ + id: 'discussion3', + resolved: true, + lineCode: 'abc_1_1', + fileHash: 'file123', + }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + }); + + describe('general discussions', () => { + it('returns general action for discussions without line_code or file_hash', () => { + const discussions = [createGeneralDiscussion({ id: 'discussion1' })]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'general', + discussionId: 'discussion1', + }); + }); + }); + + describe('edge cases', () => { + it('returns null when discussion is not found', () => { + const discussions = [createLineDiscussion({ id: 'discussion1' })]; + + expect(determineCollapseAction(discussions, 'nonexistent')).toBeNull(); + }); + + it('prioritizes line_code over file_hash when both exist', () => { + const discussions = [createLineDiscussion({ id: 'discussion1' })]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result.type).toBe('line'); + }); + + it('handles falsy values correctly for line_code and file_hash', () => { + const discussionsWithEmptyLineCode = [ + createDiscussion({ + id: 'discussion1', + line_code: '', + diff_file: { file_hash: 'file123' }, + }), + ]; + const discussionsWithEmptyFileHash = [ + createDiscussion({ + id: 'discussion2', + diff_file: { file_hash: '' }, + }), + ]; + const discussionsWithNullDiffFile = [ + createDiscussion({ + id: 'discussion3', + line_code: 'abc_1_1', + diff_file: null, + }), + ]; + const discussionsWithEmptyDiffFile = [ + createDiscussion({ + id: 'discussion4', + diff_file: {}, + }), + ]; + + expect(determineCollapseAction(discussionsWithEmptyLineCode, 'discussion1')).toEqual({ + type: 'file', + discussions: [discussionsWithEmptyLineCode[0]], + fileHash: 'file123', + }); + expect(determineCollapseAction(discussionsWithEmptyFileHash, 'discussion2')).toEqual({ + type: 'general', + discussionId: 'discussion2', + }); + expect(determineCollapseAction(discussionsWithNullDiffFile, 'discussion3')).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: undefined, + }); + expect(determineCollapseAction(discussionsWithEmptyDiffFile, 'discussion4')).toEqual({ + type: 'general', + discussionId: 'discussion4', + }); + }); + }); + }); +}); -- GitLab From 4b6fc0ed7512b772794567a31ef313e6000f63c6 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 11:45:40 -0600 Subject: [PATCH 04/15] Test the updated store actions --- .../notes/store/legacy_notes/actions_spec.js | 278 ++++++++++++++++++ 1 file changed, 278 insertions(+) diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index 238f7a6dd15b50..db4e793bccf536 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -642,6 +642,284 @@ describe('Actions Notes Store', () => { }); }); + describe('handleAutoCollapse', () => { + let diffStore; + + beforeEach(() => { + diffStore = useLegacyDiffs(); + diffStore.toggleLineDiscussions = jest.fn(); + diffStore.toggleFileDiscussion = jest.fn(); + store.toggleDiscussion = jest.fn(); + store.collapseLineDiscussions = jest.fn(); + }); + + describe('line type actions', () => { + const lineAction = { + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }; + + it('calls toggleLineDiscussions on diffs store when context is changes', () => { + store.handleAutoCollapse(lineAction, { context: 'changes' }); + + expect(diffStore.toggleLineDiscussions).toHaveBeenCalledWith({ + lineCode: 'abc_1_1', + fileHash: 'file123', + expanded: false, + }); + expect(store.collapseLineDiscussions).not.toHaveBeenCalled(); + }); + + it('calls collapseLineDiscussions when context is not changes', () => { + store.handleAutoCollapse(lineAction, { context: 'overview' }); + + expect(store.collapseLineDiscussions).toHaveBeenCalledWith('abc_1_1'); + expect(diffStore.toggleLineDiscussions).not.toHaveBeenCalled(); + }); + }); + + describe('file type actions', () => { + const fileAction = { + type: 'file', + discussions: [{ id: 'discussion1' }, { id: 'discussion2' }], + fileHash: 'file123', + }; + + it('calls toggleFileDiscussion on diffs store for each discussion when context is changes', () => { + store.handleAutoCollapse(fileAction, { context: 'changes' }); + + expect(diffStore.toggleFileDiscussion).toHaveBeenCalledTimes(2); + expect(diffStore.toggleFileDiscussion).toHaveBeenNthCalledWith(1, { + discussion: { id: 'discussion1' }, + expandedOnDiff: false, + }); + expect(diffStore.toggleFileDiscussion).toHaveBeenNthCalledWith(2, { + discussion: { id: 'discussion2' }, + expandedOnDiff: false, + }); + expect(store.toggleDiscussion).not.toHaveBeenCalled(); + }); + + it('calls toggleDiscussion for each discussion when context is not changes', () => { + store.handleAutoCollapse(fileAction, { context: 'overview' }); + + expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); + expect(store.toggleDiscussion).toHaveBeenNthCalledWith(1, { + discussionId: 'discussion1', + forceExpanded: false, + }); + expect(store.toggleDiscussion).toHaveBeenNthCalledWith(2, { + discussionId: 'discussion2', + forceExpanded: false, + }); + expect(diffStore.toggleFileDiscussion).not.toHaveBeenCalled(); + }); + }); + + describe('general type actions', () => { + const generalAction = { + type: 'general', + discussionId: 'discussion1', + }; + + it('calls toggleDiscussion regardless of context', () => { + store.handleAutoCollapse(generalAction, { context: 'changes' }); + + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion1', + forceExpanded: false, + }); + }); + }); + + describe('unknown type actions', () => { + it('handles unknown action types gracefully', () => { + const unknownAction = { type: 'unknown' }; + + expect(() => { + store.handleAutoCollapse(unknownAction, { context: 'changes' }); + }).not.toThrow(); + + expect(diffStore.toggleLineDiscussions).not.toHaveBeenCalled(); + expect(diffStore.toggleFileDiscussion).not.toHaveBeenCalled(); + expect(store.toggleDiscussion).not.toHaveBeenCalled(); + }); + }); + }); + + describe('collapseLineDiscussions', () => { + beforeEach(() => { + store.toggleDiscussion = jest.fn(); + }); + + it('collapses all discussions with matching line_code', () => { + store.discussions = [ + { id: 'discussion1', line_code: 'abc_1_1' }, + { id: 'discussion2', line_code: 'abc_1_1' }, + { id: 'discussion3', line_code: 'abc_2_2' }, + ]; + + store.collapseLineDiscussions('abc_1_1'); + + expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); + expect(store.toggleDiscussion).toHaveBeenNthCalledWith(1, { + discussionId: 'discussion1', + forceExpanded: false, + }); + expect(store.toggleDiscussion).toHaveBeenNthCalledWith(2, { + discussionId: 'discussion2', + forceExpanded: false, + }); + }); + + it('handles no matching discussions', () => { + store.discussions = [{ id: 'discussion1', line_code: 'abc_2_2' }]; + + store.collapseLineDiscussions('abc_1_1'); + + expect(store.toggleDiscussion).not.toHaveBeenCalled(); + }); + }); + + describe('getContext', () => { + let diffStore; + + beforeEach(() => { + diffStore = useLegacyDiffs(); + }); + + it('returns "changes" when diffs store has loaded diff files', () => { + diffStore.diffFiles = [{ id: 1 }, { id: 2 }]; + + const context = store.getContext(); + + expect(context).toBe('changes'); + }); + + it.each` + scenario | diffStoreSetup + ${'has no diff files'} | ${{ diffFiles: [] }} + ${'is not available'} | ${null} + ${'has undefined diffFiles'} | ${{ diffFiles: undefined }} + `('returns "overview" when diffs store $scenario', ({ diffStoreSetup }) => { + if (diffStoreSetup === null) { + jest.spyOn(store, 'tryStore').mockReturnValue(null); + } else { + Object.assign(diffStore, diffStoreSetup); + } + + const context = store.getContext(); + + expect(context).toBe('overview'); + }); + }); + + describe('toggleResolveNote with auto-collapse', () => { + const endpoint = '/discussions/discussion1/resolve'; + let diffStore; + + beforeEach(() => { + diffStore = useLegacyDiffs(); + diffStore.diffFiles = [{ id: 1 }]; + diffStore.toggleLineDiscussions = jest.fn(); + store.handleAutoCollapse = jest.fn(); + store.getContext = jest.fn().mockReturnValue('changes'); + + // We only want to test our auto-collapsing behavior and aren't worried about sub-mutations + store[types.UPDATE_DISCUSSION] = jest.fn(); + store[types.UPDATE_NOTE] = jest.fn(); + store.updateResolvableDiscussionsCounts = jest.fn(); + store.updateMergeRequestWidget = jest.fn(); + }); + + describe('when resolving a discussion', () => { + const resolvedResponse = { + id: 'discussion1', + resolved: true, + line_code: 'abc_1_1', + diff_file: { file_hash: 'file123' }, + }; + + it('triggers auto-collapse when all discussions on line are resolved', async () => { + axiosMock.onPost(endpoint).replyOnce(HTTP_STATUS_OK, resolvedResponse); + + store.discussions = [ + { + id: 'discussion1', + resolved: true, + line_code: 'abc_1_1', + diff_file: { file_hash: 'file123' }, + }, + ]; + + await store.toggleResolveNote({ + endpoint, + isResolved: false, + discussion: true, + }); + + expect(store.handleAutoCollapse).toHaveBeenCalledWith( + { + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }, + { context: 'changes' }, + ); + expect(store.getContext).toHaveBeenCalled(); + }); + + it('handles partially hydrated discussion after page refresh and still collapses', async () => { + axiosMock.onPost(endpoint).replyOnce(HTTP_STATUS_OK, resolvedResponse); + store.getContext.mockReturnValue('overview'); + + // Regression test: After page refresh on Overview tab, + // discussion exists but diff_file will be undefined. + store.discussions = [ + { + id: 'discussion1', + resolved: true, + line_code: 'abc_1_1', + diff_file: undefined, + }, + ]; + + await store.toggleResolveNote({ + endpoint, + isResolved: false, + discussion: true, + }); + + expect(store.handleAutoCollapse).toHaveBeenCalledWith( + { + type: 'line', + lineCode: 'abc_1_1', + fileHash: undefined, + }, + { context: 'overview' }, + ); + }); + }); + + describe('does not trigger auto-collapse', () => { + it.each` + scenario | method | responseEndpoint | response | discussions | params + ${'when unresolving'} | ${'onDelete'} | ${endpoint} | ${{ id: 'discussion1', resolved: false }} | ${[{ id: 'discussion1' }]} | ${{ endpoint, isResolved: true, discussion: true }} + ${'when resolving note'} | ${'onPost'} | ${endpoint} | ${{ id: 'discussion1', resolved: true }} | ${[{ id: 'discussion1' }]} | ${{ endpoint, isResolved: false, discussion: false }} + ${'invalid endpoint'} | ${'onPost'} | ${'/some/other/path'} | ${{ id: 'discussion1', resolved: true }} | ${[{ id: 'discussion1' }]} | ${{ endpoint: '/some/other/path', isResolved: false, discussion: true }} + ${'discussion not found'} | ${'onPost'} | ${endpoint} | ${{ id: 'discussion1', resolved: true }} | ${[]} | ${{ endpoint, isResolved: false, discussion: true }} + `('$scenario', async ({ method, responseEndpoint, response, discussions, params }) => { + axiosMock[method](responseEndpoint).replyOnce(HTTP_STATUS_OK, response); + store.discussions = discussions; + + await store.toggleResolveNote(params); + + expect(store.handleAutoCollapse).not.toHaveBeenCalled(); + }); + }); + }); + describe('updateMergeRequestWidget', () => { it('calls mrWidget checkStatus', () => { jest.spyOn(mrWidgetEventHub, '$emit').mockImplementation(() => {}); -- GitLab From b45499363a9edb6c1687712298a50009e951a7e7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Oct 2025 12:08:39 -0600 Subject: [PATCH 05/15] Expand discussion threads that are auto-collapsed in tests --- ...eate_issue_for_single_discussion_in_merge_request_spec.rb | 1 + .../user_resolves_diff_notes_and_discussions_resolve_spec.rb | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb index 854db4bb52ef48..60262e03adf805 100644 --- a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb @@ -43,6 +43,7 @@ def resolve_discussion_selector expect(page).to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) click_button 'Resolve thread' + expand_collapsed_discussions expect(page).not_to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 8747479e197f63..af827961a03073 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -46,6 +46,7 @@ find_by_testid('resolve-line-button').click wait_for_requests + expand_collapsed_discussions expect(find_by_testid('resolve-line-button')['aria-label']).to eq("Resolved by #{user.name}") end @@ -74,6 +75,7 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + expand_collapsed_discussions click_button 'Reopen thread' end @@ -233,6 +235,7 @@ resolve_button.click wait_for_requests + expand_collapsed_discussions expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end @@ -256,6 +259,7 @@ first('[data-testid="resolve-line-button"]').click wait_for_requests + expand_collapsed_discussions expect(first('[data-testid="resolve-line-button"]')['aria-label']).to eq("Resolved by #{user.name}") end @@ -272,6 +276,7 @@ end wait_for_requests + expand_collapsed_discussions resolve_buttons.each do |button| expect(button['aria-label']).to eq("Resolved by #{user.name}") -- GitLab From 4b8a5b0d7549880ad483c2b5795d801cda178a94 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Oct 2025 15:07:24 -0600 Subject: [PATCH 06/15] Make sure discussion expansion helpers exist everywhere they're needed --- ...e_issue_for_single_discussion_in_merge_request_spec.rb | 2 ++ ...er_resolves_diff_notes_and_discussions_resolve_spec.rb | 2 ++ spec/support/helpers/features/notes_helpers.rb | 8 ++++++++ 3 files changed, 12 insertions(+) diff --git a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb index 60262e03adf805..2f7d775f0dcff6 100644 --- a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Resolve an open thread in a merge request by creating an issue', :js, feature_category: :team_planning do + include Features::NotesHelpers + let(:user) { create(:user) } let(:project) { create(:project, :repository, only_allow_merge_if_all_discussions_are_resolved: true) } let(:merge_request) { create(:merge_request, source_project: project) } diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index af827961a03073..962c9ef83bb528 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Merge request > User resolves diff notes and threads', :js, feature_category: :code_review_workflow do + include MergeRequestDiffHelpers + let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } let(:guest) { create(:user) } diff --git a/spec/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index 55c1a29bef84d9..9263c5c5442d8d 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -53,5 +53,13 @@ def preview_note(text, issuable_type = nil) yield if block_given? end end + + def expand_collapsed_discussions + first('.js-diff-comment-avatar').click + end + + def expand_all_collapsed_discussions + all('.js-diff-comment-avatar').each(&:click) + end end end -- GitLab From 0fdb3a3d3f673f18393c64470e878d9ffe8dbc1b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 18:33:09 -0600 Subject: [PATCH 07/15] Fix stale element issues --- ...sue_for_single_discussion_in_merge_request_spec.rb | 2 +- ...esolves_diff_notes_and_discussions_resolve_spec.rb | 10 +++++----- .../user_suggests_changes_on_diff_spec.rb | 8 ++++---- spec/support/helpers/features/notes_helpers.rb | 11 ++++++----- spec/support/helpers/merge_request_diff_helpers.rb | 11 ++++++----- 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb index 2f7d775f0dcff6..fdd5c9ead73c56 100644 --- a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb @@ -45,7 +45,7 @@ def resolve_discussion_selector expect(page).to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) click_button 'Resolve thread' - expand_collapsed_discussions + expand_all_collapsed_discussions expect(page).not_to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 962c9ef83bb528..8c9c7440db7dfa 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -48,7 +48,7 @@ find_by_testid('resolve-line-button').click wait_for_requests - expand_collapsed_discussions + expand_all_collapsed_discussions expect(find_by_testid('resolve-line-button')['aria-label']).to eq("Resolved by #{user.name}") end @@ -77,7 +77,7 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click - expand_collapsed_discussions + expand_all_collapsed_discussions click_button 'Reopen thread' end @@ -237,7 +237,7 @@ resolve_button.click wait_for_requests - expand_collapsed_discussions + expand_all_collapsed_discussions expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end @@ -261,7 +261,7 @@ first('[data-testid="resolve-line-button"]').click wait_for_requests - expand_collapsed_discussions + expand_all_collapsed_discussions expect(first('[data-testid="resolve-line-button"]')['aria-label']).to eq("Resolved by #{user.name}") end @@ -278,7 +278,7 @@ end wait_for_requests - expand_collapsed_discussions + expand_all_collapsed_discussions resolve_buttons.each do |button| expect(button['aria-label']).to eq("Resolved by #{user.name}") diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index 9a2ec37d5b3b37..b7aa8ecadf2c94 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -107,7 +107,7 @@ def expect_suggestion_has_content(element, expected_changing_content, expected_s wait_for_requests end - expand_collapsed_discussions + expand_all_collapsed_discussions page.within('.diff-discussions') do expect(page).to have_content('Applied') @@ -280,7 +280,7 @@ def hash(path) wait_for_requests end - expand_collapsed_discussions + expand_all_collapsed_discussions page.within(container) do expect(page).to have_content('Applied').once @@ -404,7 +404,7 @@ def hash(path) wait_for_requests end - expand_collapsed_discussions + expand_all_collapsed_discussions page.within("[id='#{hash}']") do expect(page).to have_content('Applied') @@ -420,7 +420,7 @@ def hash(path) wait_for_requests end - expand_collapsed_discussions + expand_all_collapsed_discussions page.within("[id='#{hash}']") do expect(page).to have_content('Reopen thread') diff --git a/spec/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index 9263c5c5442d8d..e9e41f691339e3 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -54,12 +54,13 @@ def preview_note(text, issuable_type = nil) end end - def expand_collapsed_discussions - first('.js-diff-comment-avatar').click - end - def expand_all_collapsed_discussions - all('.js-diff-comment-avatar').each(&:click) + count = all('.js-diff-comment-avatar').size + count.times do + break unless has_selector?('.js-diff-comment-avatar') + + first('.js-diff-comment-avatar').click + end end end end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index dd296bba31bda8..d6e67f2f847ca1 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -94,11 +94,12 @@ def find_by_scrolling(selector, **options) element end - def expand_collapsed_discussions - first('.js-diff-comment-avatar').click - end - def expand_all_collapsed_discussions - all('.js-diff-comment-avatar').each(&:click) + count = all('.js-diff-comment-avatar').size + count.times do + break unless has_selector?('.js-diff-comment-avatar') + + first('.js-diff-comment-avatar').click + end end end -- GitLab From 62489190d704927da36468b7632434a3ea89f7b0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 20:42:32 -0600 Subject: [PATCH 08/15] Add diff-specific auto-collapse actions --- .../diffs/stores/legacy_diffs/actions.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index a05be1f97671c7..2b553d160d0967 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -23,6 +23,10 @@ import { lineExists, } from '~/diffs/utils/diff_file'; import { useNotes } from '~/notes/store/legacy_notes'; +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '~/notes/utils/discussion_collapse'; import { INLINE_DIFF_VIEW_TYPE, DIFF_VIEW_COOKIE_NAME, @@ -1270,3 +1274,55 @@ export function expandAllFiles() { export function collapseAllFiles() { this[types.SET_COLLAPSED_STATE_FOR_ALL_FILES]({ collapsed: true }); } + +/** + * Resolves a discussion with auto-collapse behavior for diffs context + * Reuses the core resolve logic from notes store, then applies diff-specific collapse + * @param {Object} params - { endpoint, isResolved, discussion } + */ +export function resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }) { + return useNotes() + .toggleResolveNote({ endpoint, isResolved, discussion }) + .then((result) => { + if (!isResolved && discussion) { + this.handleDiffAutoCollapse({ endpoint }); + } + return result; + }); +} + +/** + * Handles auto-collapse logic specific to diffs context + * @param {Object} params - { endpoint } + */ +export function handleDiffAutoCollapse({ endpoint }) { + const discussionId = extractDiscussionIdFromEndpoint(endpoint); + if (!discussionId) return; + + const { discussions } = useNotes(); + const collapseAction = determineCollapseAction(discussions, discussionId); + if (!collapseAction) return; + + const actionHandlers = { + line: (action) => { + this.toggleLineDiscussions({ + lineCode: action.lineCode, + fileHash: action.fileHash, + expanded: false, + }); + }, + file: (action) => { + action.discussions.forEach((discussion) => { + this.toggleFileDiscussion({ + discussion, + expandedOnDiff: false, + }); + }); + }, + }; + + const handler = actionHandlers[collapseAction.type]; + if (handler) { + handler(collapseAction); + } +} -- GitLab From c9a42c7f69e6b12a04ad02870278b344b3a98557 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 21:29:35 -0600 Subject: [PATCH 09/15] Continue trying to fix issues with stale elements --- spec/support/helpers/merge_request_diff_helpers.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index d6e67f2f847ca1..014fa346812356 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -95,9 +95,8 @@ def find_by_scrolling(selector, **options) end def expand_all_collapsed_discussions - count = all('.js-diff-comment-avatar').size - count.times do - break unless has_selector?('.js-diff-comment-avatar') + loop do + break unless has_selector?('.js-diff-comment-avatar', wait: 1) first('.js-diff-comment-avatar').click end -- GitLab From 8a93f0428f6e5f7ebe6d40cce809dea0fc781e9b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 23:18:40 -0600 Subject: [PATCH 10/15] Handle discussion collapsing at the component level --- .../notes/components/noteable_discussion.vue | 38 +++++++++++++ .../notes/store/legacy_notes/actions.js | 53 +++---------------- 2 files changed, 45 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 267e22e00afb44..fd415a4bcd7c73 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -220,6 +220,9 @@ export default { 'expandDiscussion', ]), ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), + resolveDiscussionWithAutoCollapse() { + return useLegacyDiffs().resolveDiscussionWithAutoCollapse(); + }, showReplyForm(text) { this.isReplying = true; @@ -318,6 +321,41 @@ export default { this.showReplyForm(); } }, + // Override resolvable mixin's handler to use component-layer decision + resolveHandler(resolvedState = false) { + if (this.note && this.note.isDraft) { + return this.$emit('toggleResolveStatus'); + } + + this.isResolving = true; + const isResolved = this.discussionResolved || resolvedState; + const discussion = this.resolveAsThread; + let endpoint = + discussion && this.discussion ? this.discussion.resolve_path : `${this.note.path}/resolve`; + + if (this.discussionResolvePath) { + endpoint = this.discussionResolvePath; + } + + // Use diffs store for diffs tab, notes store for overview tab + const resolveAction = this.isOverviewTab + ? this.toggleResolveNote + : this.resolveDiscussionWithAutoCollapse; + + return resolveAction({ endpoint, isResolved, discussion }) + .then(() => { + this.isResolving = false; + }) + .catch(() => { + this.isResolving = false; + + const msg = __('Something went wrong while resolving this discussion. Please try again.'); + createAlert({ + message: msg, + parent: this.$el, + }); + }); + }, }, }; diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 9daad449577631..07340fe34399ba 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -23,10 +23,6 @@ import { TYPENAME_NOTE } from '~/graphql_shared/constants'; import { useBatchComments } from '~/batch_comments/store'; import { uuids } from '~/lib/utils/uuids'; import notesEventHub from '../../event_hub'; -import { - extractDiscussionIdFromEndpoint, - determineCollapseAction, -} from '../../utils/discussion_collapse'; import promoteTimelineEvent from '../../graphql/promote_timeline_event.mutation.graphql'; @@ -426,50 +422,22 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this.updateResolvableDiscussionsCounts(); this.updateMergeRequestWidget(); - - const discussionId = extractDiscussionIdFromEndpoint(endpoint); - if (discussionId && !isResolved && discussion) { - const collapseAction = determineCollapseAction(this.discussions, discussionId); - if (collapseAction) { - const context = this.getContext(); - this.handleAutoCollapse(collapseAction, { context }); - } - } }); } -export function handleAutoCollapse(collapseAction, options = {}) { - const { context } = options; - +export function handleAutoCollapse(collapseAction) { const actionHandlers = { line: (action) => { - if (context === 'changes') { - this.tryStore('legacyDiffs').toggleLineDiscussions({ - lineCode: action.lineCode, - fileHash: action.fileHash, - expanded: false, - }); - } else { - this.collapseLineDiscussions(action.lineCode); - } + this.collapseLineDiscussions(action.lineCode); }, file: (action) => { - if (context === 'changes') { - action.discussions.forEach((discussion) => { - this.tryStore('legacyDiffs').toggleFileDiscussion({ - discussion, - expandedOnDiff: false, - }); + action.discussions.forEach((discussion) => { + this.toggleDiscussion({ + discussionId: discussion.id, + forceExpanded: false, }); - } else { - action.discussions.forEach((discussion) => { - this.toggleDiscussion({ - discussionId: discussion.id, - forceExpanded: false, - }); - }); - } + }); }, general: (action) => { @@ -494,13 +462,6 @@ export function collapseLineDiscussions(lineCode) { }); } -export function getContext() { - const diffStore = this.tryStore('legacyDiffs'); - const hasLoadedDiffs = diffStore && diffStore.diffFiles && diffStore.diffFiles.length > 0; - - return hasLoadedDiffs ? 'changes' : 'overview'; -} - export function closeIssuable() { this.toggleStateButtonLoading(true); return axios.put(this.notesData.closePath).then(({ data }) => { -- GitLab From 979087167a224c30359920c338bd84361a5fdb31 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 23:31:59 -0600 Subject: [PATCH 11/15] Still trying to get stale elements to stop breaking tests --- spec/support/helpers/features/notes_helpers.rb | 14 +++++++++----- spec/support/helpers/merge_request_diff_helpers.rb | 13 +++++++++---- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/spec/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index e9e41f691339e3..1c1e2d7dd4ead1 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -55,11 +55,15 @@ def preview_note(text, issuable_type = nil) end def expand_all_collapsed_discussions - count = all('.js-diff-comment-avatar').size - count.times do - break unless has_selector?('.js-diff-comment-avatar') - - first('.js-diff-comment-avatar').click + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('.js-diff-comment-avatar', wait: 1) + begin + first('.js-diff-comment-avatar', wait: 1, minimum: 1).click + wait_for_requests + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end end end end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index 014fa346812356..ec2df60725f056 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -95,10 +95,15 @@ def find_by_scrolling(selector, **options) end def expand_all_collapsed_discussions - loop do - break unless has_selector?('.js-diff-comment-avatar', wait: 1) - - first('.js-diff-comment-avatar').click + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('.js-diff-comment-avatar', wait: 1) + begin + first('.js-diff-comment-avatar', wait: 1, minimum: 1).click + wait_for_requests + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end end end end -- GitLab From 42a84daa4a3128b6144a37e66c2526499957e079 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 23:44:39 -0600 Subject: [PATCH 12/15] Update tests with new methods and signatures --- .../notes/store/legacy_notes/actions_spec.js | 254 ++---------------- 1 file changed, 21 insertions(+), 233 deletions(-) diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index db4e793bccf536..cce19c35841d04 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -643,108 +643,42 @@ describe('Actions Notes Store', () => { }); describe('handleAutoCollapse', () => { - let diffStore; - beforeEach(() => { - diffStore = useLegacyDiffs(); - diffStore.toggleLineDiscussions = jest.fn(); - diffStore.toggleFileDiscussion = jest.fn(); store.toggleDiscussion = jest.fn(); store.collapseLineDiscussions = jest.fn(); }); - describe('line type actions', () => { - const lineAction = { - type: 'line', - lineCode: 'abc_1_1', - fileHash: 'file123', - }; - - it('calls toggleLineDiscussions on diffs store when context is changes', () => { - store.handleAutoCollapse(lineAction, { context: 'changes' }); - - expect(diffStore.toggleLineDiscussions).toHaveBeenCalledWith({ - lineCode: 'abc_1_1', - fileHash: 'file123', - expanded: false, - }); - expect(store.collapseLineDiscussions).not.toHaveBeenCalled(); - }); - - it('calls collapseLineDiscussions when context is not changes', () => { - store.handleAutoCollapse(lineAction, { context: 'overview' }); + it.each` + type | action | expectedCall + ${'line'} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${['collapseLineDiscussions', 'abc_1_1']} + ${'general'} | ${{ type: 'general', discussionId: 'discussion1' }} | ${['toggleDiscussion', { discussionId: 'discussion1', forceExpanded: false }]} + `('handles $type action type', ({ action, expectedCall }) => { + store.handleAutoCollapse(action); - expect(store.collapseLineDiscussions).toHaveBeenCalledWith('abc_1_1'); - expect(diffStore.toggleLineDiscussions).not.toHaveBeenCalled(); - }); + expect(store[expectedCall[0]]).toHaveBeenCalledWith(expectedCall[1]); }); - describe('file type actions', () => { + it('handles file action type with multiple discussions', () => { const fileAction = { type: 'file', discussions: [{ id: 'discussion1' }, { id: 'discussion2' }], - fileHash: 'file123', }; - it('calls toggleFileDiscussion on diffs store for each discussion when context is changes', () => { - store.handleAutoCollapse(fileAction, { context: 'changes' }); + store.handleAutoCollapse(fileAction); - expect(diffStore.toggleFileDiscussion).toHaveBeenCalledTimes(2); - expect(diffStore.toggleFileDiscussion).toHaveBeenNthCalledWith(1, { - discussion: { id: 'discussion1' }, - expandedOnDiff: false, - }); - expect(diffStore.toggleFileDiscussion).toHaveBeenNthCalledWith(2, { - discussion: { id: 'discussion2' }, - expandedOnDiff: false, - }); - expect(store.toggleDiscussion).not.toHaveBeenCalled(); - }); - - it('calls toggleDiscussion for each discussion when context is not changes', () => { - store.handleAutoCollapse(fileAction, { context: 'overview' }); - - expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); - expect(store.toggleDiscussion).toHaveBeenNthCalledWith(1, { - discussionId: 'discussion1', - forceExpanded: false, - }); - expect(store.toggleDiscussion).toHaveBeenNthCalledWith(2, { - discussionId: 'discussion2', - forceExpanded: false, - }); - expect(diffStore.toggleFileDiscussion).not.toHaveBeenCalled(); - }); - }); - - describe('general type actions', () => { - const generalAction = { - type: 'general', + expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ discussionId: 'discussion1', - }; - - it('calls toggleDiscussion regardless of context', () => { - store.handleAutoCollapse(generalAction, { context: 'changes' }); - - expect(store.toggleDiscussion).toHaveBeenCalledWith({ - discussionId: 'discussion1', - forceExpanded: false, - }); + forceExpanded: false, + }); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion2', + forceExpanded: false, }); }); - describe('unknown type actions', () => { - it('handles unknown action types gracefully', () => { - const unknownAction = { type: 'unknown' }; - - expect(() => { - store.handleAutoCollapse(unknownAction, { context: 'changes' }); - }).not.toThrow(); - - expect(diffStore.toggleLineDiscussions).not.toHaveBeenCalled(); - expect(diffStore.toggleFileDiscussion).not.toHaveBeenCalled(); - expect(store.toggleDiscussion).not.toHaveBeenCalled(); - }); + it('handles unknown action types gracefully', () => { + expect(() => store.handleAutoCollapse({ type: 'unknown' })).not.toThrow(); }); }); @@ -753,7 +687,7 @@ describe('Actions Notes Store', () => { store.toggleDiscussion = jest.fn(); }); - it('collapses all discussions with matching line_code', () => { + it('collapses only discussions matching the line_code', () => { store.discussions = [ { id: 'discussion1', line_code: 'abc_1_1' }, { id: 'discussion2', line_code: 'abc_1_1' }, @@ -763,161 +697,15 @@ describe('Actions Notes Store', () => { store.collapseLineDiscussions('abc_1_1'); expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); - expect(store.toggleDiscussion).toHaveBeenNthCalledWith(1, { + expect(store.toggleDiscussion).toHaveBeenCalledWith({ discussionId: 'discussion1', forceExpanded: false, }); - expect(store.toggleDiscussion).toHaveBeenNthCalledWith(2, { + expect(store.toggleDiscussion).toHaveBeenCalledWith({ discussionId: 'discussion2', forceExpanded: false, }); }); - - it('handles no matching discussions', () => { - store.discussions = [{ id: 'discussion1', line_code: 'abc_2_2' }]; - - store.collapseLineDiscussions('abc_1_1'); - - expect(store.toggleDiscussion).not.toHaveBeenCalled(); - }); - }); - - describe('getContext', () => { - let diffStore; - - beforeEach(() => { - diffStore = useLegacyDiffs(); - }); - - it('returns "changes" when diffs store has loaded diff files', () => { - diffStore.diffFiles = [{ id: 1 }, { id: 2 }]; - - const context = store.getContext(); - - expect(context).toBe('changes'); - }); - - it.each` - scenario | diffStoreSetup - ${'has no diff files'} | ${{ diffFiles: [] }} - ${'is not available'} | ${null} - ${'has undefined diffFiles'} | ${{ diffFiles: undefined }} - `('returns "overview" when diffs store $scenario', ({ diffStoreSetup }) => { - if (diffStoreSetup === null) { - jest.spyOn(store, 'tryStore').mockReturnValue(null); - } else { - Object.assign(diffStore, diffStoreSetup); - } - - const context = store.getContext(); - - expect(context).toBe('overview'); - }); - }); - - describe('toggleResolveNote with auto-collapse', () => { - const endpoint = '/discussions/discussion1/resolve'; - let diffStore; - - beforeEach(() => { - diffStore = useLegacyDiffs(); - diffStore.diffFiles = [{ id: 1 }]; - diffStore.toggleLineDiscussions = jest.fn(); - store.handleAutoCollapse = jest.fn(); - store.getContext = jest.fn().mockReturnValue('changes'); - - // We only want to test our auto-collapsing behavior and aren't worried about sub-mutations - store[types.UPDATE_DISCUSSION] = jest.fn(); - store[types.UPDATE_NOTE] = jest.fn(); - store.updateResolvableDiscussionsCounts = jest.fn(); - store.updateMergeRequestWidget = jest.fn(); - }); - - describe('when resolving a discussion', () => { - const resolvedResponse = { - id: 'discussion1', - resolved: true, - line_code: 'abc_1_1', - diff_file: { file_hash: 'file123' }, - }; - - it('triggers auto-collapse when all discussions on line are resolved', async () => { - axiosMock.onPost(endpoint).replyOnce(HTTP_STATUS_OK, resolvedResponse); - - store.discussions = [ - { - id: 'discussion1', - resolved: true, - line_code: 'abc_1_1', - diff_file: { file_hash: 'file123' }, - }, - ]; - - await store.toggleResolveNote({ - endpoint, - isResolved: false, - discussion: true, - }); - - expect(store.handleAutoCollapse).toHaveBeenCalledWith( - { - type: 'line', - lineCode: 'abc_1_1', - fileHash: 'file123', - }, - { context: 'changes' }, - ); - expect(store.getContext).toHaveBeenCalled(); - }); - - it('handles partially hydrated discussion after page refresh and still collapses', async () => { - axiosMock.onPost(endpoint).replyOnce(HTTP_STATUS_OK, resolvedResponse); - store.getContext.mockReturnValue('overview'); - - // Regression test: After page refresh on Overview tab, - // discussion exists but diff_file will be undefined. - store.discussions = [ - { - id: 'discussion1', - resolved: true, - line_code: 'abc_1_1', - diff_file: undefined, - }, - ]; - - await store.toggleResolveNote({ - endpoint, - isResolved: false, - discussion: true, - }); - - expect(store.handleAutoCollapse).toHaveBeenCalledWith( - { - type: 'line', - lineCode: 'abc_1_1', - fileHash: undefined, - }, - { context: 'overview' }, - ); - }); - }); - - describe('does not trigger auto-collapse', () => { - it.each` - scenario | method | responseEndpoint | response | discussions | params - ${'when unresolving'} | ${'onDelete'} | ${endpoint} | ${{ id: 'discussion1', resolved: false }} | ${[{ id: 'discussion1' }]} | ${{ endpoint, isResolved: true, discussion: true }} - ${'when resolving note'} | ${'onPost'} | ${endpoint} | ${{ id: 'discussion1', resolved: true }} | ${[{ id: 'discussion1' }]} | ${{ endpoint, isResolved: false, discussion: false }} - ${'invalid endpoint'} | ${'onPost'} | ${'/some/other/path'} | ${{ id: 'discussion1', resolved: true }} | ${[{ id: 'discussion1' }]} | ${{ endpoint: '/some/other/path', isResolved: false, discussion: true }} - ${'discussion not found'} | ${'onPost'} | ${endpoint} | ${{ id: 'discussion1', resolved: true }} | ${[]} | ${{ endpoint, isResolved: false, discussion: true }} - `('$scenario', async ({ method, responseEndpoint, response, discussions, params }) => { - axiosMock[method](responseEndpoint).replyOnce(HTTP_STATUS_OK, response); - store.discussions = discussions; - - await store.toggleResolveNote(params); - - expect(store.handleAutoCollapse).not.toHaveBeenCalled(); - }); - }); }); describe('updateMergeRequestWidget', () => { -- GitLab From 0f63decfc2772c2905bc17c1bab20e10158203bb Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Oct 2025 00:31:50 -0600 Subject: [PATCH 13/15] Remove unused no-unused-properties eslint-disable directives --- app/assets/javascripts/notes/components/noteable_discussion.vue | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index fd415a4bcd7c73..dffef5f24ffc32 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -83,7 +83,6 @@ export default { return { isReplying: false, isResolving: false, - // eslint-disable-next-line vue/no-unused-properties -- `resolveAsThread` is used by the `resolvable` mixin resolveAsThread: true, }; }, @@ -214,7 +213,6 @@ export default { ...mapActions(useNotes, [ 'saveNote', 'removePlaceholderNotes', - // eslint-disable-next-line vue/no-unused-properties -- toggleResolveNote() used by the `Resolvable` mixin 'toggleResolveNote', 'removeConvertedDiscussion', 'expandDiscussion', -- GitLab From 972a1ffc6fe7cfc1c7e3c57564bf0a443e8f85bf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Oct 2025 00:45:34 -0600 Subject: [PATCH 14/15] Fix missing argument forwarding --- .../javascripts/notes/components/noteable_discussion.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index dffef5f24ffc32..26daae6f208a9b 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -218,8 +218,8 @@ export default { 'expandDiscussion', ]), ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), - resolveDiscussionWithAutoCollapse() { - return useLegacyDiffs().resolveDiscussionWithAutoCollapse(); + resolveDiscussionWithAutoCollapse(...args) { + return useLegacyDiffs().resolveDiscussionWithAutoCollapse(...args); }, showReplyForm(text) { this.isReplying = true; -- GitLab From 404474b20e7b1d142e6b07a9044feecae68bc885 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 23 Oct 2025 08:14:38 -0600 Subject: [PATCH 15/15] Check if an infinite collapse loop is crashing the test browser --- spec/support/helpers/features/notes_helpers.rb | 6 +++++- spec/support/helpers/merge_request_diff_helpers.rb | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/spec/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index 1c1e2d7dd4ead1..869e6f9bce3848 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -55,11 +55,15 @@ def preview_note(text, issuable_type = nil) end def expand_all_collapsed_discussions + max_expansions = 50 + expansions = 0 + allowing_for_delay(interval: 0.5, retries: 3) do - while has_selector?('.js-diff-comment-avatar', wait: 1) + while has_selector?('.js-diff-comment-avatar', wait: 1) && expansions < max_expansions begin first('.js-diff-comment-avatar', wait: 1, minimum: 1).click wait_for_requests + expansions += 1 rescue Selenium::WebDriver::Error::StaleElementReferenceError raise Capybara::ExpectationNotMet end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index ec2df60725f056..a24da0faee73ad 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -95,11 +95,15 @@ def find_by_scrolling(selector, **options) end def expand_all_collapsed_discussions + max_expansions = 50 + expansions = 0 + allowing_for_delay(interval: 0.5, retries: 3) do - while has_selector?('.js-diff-comment-avatar', wait: 1) + while has_selector?('.js-diff-comment-avatar', wait: 1) && expansions < max_expansions begin first('.js-diff-comment-avatar', wait: 1, minimum: 1).click wait_for_requests + expansions += 1 rescue Selenium::WebDriver::Error::StaleElementReferenceError raise Capybara::ExpectationNotMet end -- GitLab