From d3cca778e787178d76e42447e624906fa9897c64 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 01:14:19 -0600 Subject: [PATCH 01/32] Auto-collapse a discussion if all threads are resolved Changelog: added --- .../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 953694be5a70742966f181b013a73b2af24537e4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Jul 2025 20:38:14 -0600 Subject: [PATCH 02/32] 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 3da83424b1fd15..9f522e7f587069 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 ec9e42723af389..ca877ff13b5ec6 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -153,4 +153,12 @@ def find_in_panel_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 9287c3ceed4ffccfb25adc6f5e16f778af4b1215 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 10:43:59 -0600 Subject: [PATCH 03/32] 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 a6038e412afd44389a5923f11074b2bfc144f2c1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 17 Oct 2025 11:45:40 -0600 Subject: [PATCH 04/32] 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 ced1fb22a1884b82ff0b7cd0f8249d550460e305 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Oct 2025 12:08:39 -0600 Subject: [PATCH 05/32] 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 ca2072fdc6b5a4e9ddf8bbf980a98ec80bb4fb61 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Oct 2025 15:07:24 -0600 Subject: [PATCH 06/32] 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 f6c0cef1b532b13e2ad1699fc8869b94528643dd Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 18:33:09 -0600 Subject: [PATCH 07/32] Fix stale element issues --- .../diffs/components/diff_gutter_avatars.vue | 1 + qa/qa/page/merge_request/show.rb | 8 +++++++- ...for_single_discussion_in_merge_request_spec.rb | 2 +- ...ves_diff_notes_and_discussions_resolve_spec.rb | 10 +++++----- .../user_suggests_changes_on_diff_spec.rb | 8 ++++---- spec/support/helpers/features/notes_helpers.rb | 15 ++++++++++----- .../support/helpers/merge_request_diff_helpers.rb | 15 ++++++++++----- 7 files changed, 38 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue index 60d985edd33a2c..5ecf90eaa048cd 100644 --- a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue +++ b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue @@ -85,6 +85,7 @@ export default { pseudo class="diff-comment-avatar js-diff-comment-avatar" css-classes="gl-bg-default" + data-testid="discussion-avatar" @click.native="$emit('toggleLineDiscussions')" /> Date: Tue, 21 Oct 2025 20:42:32 -0600 Subject: [PATCH 08/32] 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 b96b679c817c98561acf1b37a8a81c89609ad5c3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 23:18:40 -0600 Subject: [PATCH 09/32] 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 708740ccd1decd..c752d92ad4b8d2 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -221,6 +221,9 @@ export default { 'expandDiscussion', ]), ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), + resolveDiscussionWithAutoCollapse() { + return useLegacyDiffs().resolveDiscussionWithAutoCollapse(); + }, showReplyForm(text) { this.isReplying = true; @@ -326,6 +329,41 @@ export default { this.showReplyForm(text); }); }, + // 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 4340027287652881517c4f933912c089ea73ab2f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Oct 2025 23:44:39 -0600 Subject: [PATCH 10/32] 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 83ff151f22dd3c5a9770e5777607245040ad3f6c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Oct 2025 00:31:50 -0600 Subject: [PATCH 11/32] 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 c752d92ad4b8d2..b594719091cb89 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -84,7 +84,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, }; }, @@ -215,7 +214,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 9893739706874425ece573a0de43522d03253792 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Oct 2025 00:45:34 -0600 Subject: [PATCH 12/32] 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 b594719091cb89..79925c6b083be0 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -219,8 +219,8 @@ export default { 'expandDiscussion', ]), ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), - resolveDiscussionWithAutoCollapse() { - return useLegacyDiffs().resolveDiscussionWithAutoCollapse(); + resolveDiscussionWithAutoCollapse(...args) { + return useLegacyDiffs().resolveDiscussionWithAutoCollapse(...args); }, showReplyForm(text) { this.isReplying = true; -- GitLab From 6525bc47039377bd96f472cf3d25a9bfb9a50634 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Oct 2025 12:35:33 -0600 Subject: [PATCH 13/32] Collapse threads when they are resolved by applying a suggestion --- .../notes/components/note_body.vue | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 58cfd8dcd06c55..11f89bcef0bf73 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -8,6 +8,10 @@ import { renderGFM } from '~/behaviors/markdown/render_gfm'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useMrNotes } from '~/mr_notes/store/legacy_mr_notes'; import { useNotes } from '~/notes/store/legacy_notes'; +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '~/notes/utils/discussion_collapse'; import NoteAttachment from './note_attachment.vue'; import NoteAwardsList from './note_awards_list.vue'; import NoteEditedText from './note_edited_text.vue'; @@ -164,7 +168,9 @@ export default { 'submitSuggestionBatch', 'addSuggestionInfoToBatch', 'removeSuggestionInfoFromBatch', + 'handleAutoCollapse', ]), + ...mapActions(useLegacyDiffs, ['handleDiffAutoCollapse']), renderGFM() { renderGFM(this.$refs['note-body']); }, @@ -175,6 +181,30 @@ export default { formCancelHandler(shouldConfirm, isDirty) { this.$emit('cancelForm', { shouldConfirm, isDirty }); }, + triggerAutoCollapse(discussionId) { + const discussion = this.getDiscussion(discussionId); + if (!discussion?.resolve_path) return; + + const diffsStore = useLegacyDiffs(); + const diffFileLoaded = diffsStore.diffFiles.some( + f => f.file_hash === discussion.diff_file?.file_hash + ); + + if (diffFileLoaded) { + this.handleDiffAutoCollapse({ endpoint: discussion.resolve_path }); + } else { + const resolvedDiscussionId = extractDiscussionIdFromEndpoint(discussion.resolve_path); + if (!resolvedDiscussionId) return; + + const collapseAction = determineCollapseAction( + useNotes().discussions, + resolvedDiscussionId + ); + if (collapseAction) { + this.handleAutoCollapse(collapseAction); + } + } + }, applySuggestion({ suggestionId, flashContainer, callback = () => {}, message }) { const { discussion_id: discussionId, id: noteId } = this.note; @@ -184,10 +214,25 @@ export default { suggestionId, flashContainer, message, - }).then(callback); + }).then((result) => { + if (this.isDiffNote) { + this.triggerAutoCollapse(discussionId); + } + callback(); + return result; + }); }, applySuggestionBatch({ message, flashContainer }) { - return this.submitSuggestionBatch({ message, flashContainer }); + const batchInfo = [...this.batchSuggestionsInfo]; + + return this.submitSuggestionBatch({ message, flashContainer }).then((result) => { + if (this.isDiffNote) { + batchInfo.forEach(({ discussionId }) => { + this.triggerAutoCollapse(discussionId); + }); + } + return result; + }); }, addSuggestionToBatch(suggestionId) { const { discussion_id: discussionId, id: noteId } = this.note; -- GitLab From 1fccae97a23dc6df8b5b65a1910f3ab52feeba16 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Oct 2025 12:37:37 -0600 Subject: [PATCH 14/32] Slightly simplify the decision branching with a helper --- .../notes/components/note_body.vue | 26 ++++++++----------- .../notes/utils/discussion_collapse.js | 11 ++++++++ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 11f89bcef0bf73..88b5261133ba35 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -8,10 +8,7 @@ import { renderGFM } from '~/behaviors/markdown/render_gfm'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useMrNotes } from '~/mr_notes/store/legacy_mr_notes'; import { useNotes } from '~/notes/store/legacy_notes'; -import { - extractDiscussionIdFromEndpoint, - determineCollapseAction, -} from '~/notes/utils/discussion_collapse'; +import { getCollapseActionFromEndpoint } from '~/notes/utils/discussion_collapse'; import NoteAttachment from './note_attachment.vue'; import NoteAwardsList from './note_awards_list.vue'; import NoteEditedText from './note_edited_text.vue'; @@ -187,22 +184,21 @@ export default { const diffsStore = useLegacyDiffs(); const diffFileLoaded = diffsStore.diffFiles.some( - f => f.file_hash === discussion.diff_file?.file_hash + (f) => f.file_hash === discussion.diff_file?.file_hash, ); if (diffFileLoaded) { this.handleDiffAutoCollapse({ endpoint: discussion.resolve_path }); - } else { - const resolvedDiscussionId = extractDiscussionIdFromEndpoint(discussion.resolve_path); - if (!resolvedDiscussionId) return; + return; + } - const collapseAction = determineCollapseAction( - useNotes().discussions, - resolvedDiscussionId - ); - if (collapseAction) { - this.handleAutoCollapse(collapseAction); - } + const collapseAction = getCollapseActionFromEndpoint( + discussion.resolve_path, + useNotes().discussions, + ); + + if (collapseAction) { + this.handleAutoCollapse(collapseAction); } }, applySuggestion({ suggestionId, flashContainer, callback = () => {}, message }) { diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js index a1ba3d11cbb0b9..317ba94233ccde 100644 --- a/app/assets/javascripts/notes/utils/discussion_collapse.js +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -65,3 +65,14 @@ export function determineCollapseAction(discussions, resolvedDiscussionId) { return collapseAction; } + +/** + * Gets collapse action from endpoint and discussions list + * @param {string} endpoint - The resolve endpoint URL + * @param {Array} discussions - Array of all discussions + * @returns {Object|null} - Collapse action or null + */ +export function getCollapseActionFromEndpoint(endpoint, discussions) { + const discussionId = extractDiscussionIdFromEndpoint(endpoint); + return discussionId ? determineCollapseAction(discussions, discussionId) : null; +} -- GitLab From 6fe5dbda9abe0935775064967b3b21996f5e64dc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Oct 2025 13:02:58 -0600 Subject: [PATCH 15/32] Unify all `toggleFileDiscussion` calls to the correct interface --- app/assets/javascripts/diffs/components/diff_discussions.vue | 2 +- app/assets/javascripts/diffs/components/diff_file.vue | 2 +- app/assets/javascripts/diffs/mixins/image_diff.js | 2 +- app/assets/javascripts/diffs/stores/legacy_diffs/actions.js | 4 ++-- ee/app/assets/javascripts/diffs/mixins/image_diff.js | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index d1a773eaf84c69..b35c0a784d8968 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -43,7 +43,7 @@ export default { return this.shouldCollapseDiscussions ? discussion.expandedOnDiff : true; }, toggleVisibility(discussion) { - this.toggleFileDiscussion(discussion); + this.toggleFileDiscussion({ discussion }); }, }, }; diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index ac804eec1c8af7..e18b5b15cdadea 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -444,7 +444,7 @@ export default { this.addToReview(note, this.$options.FILE_DIFF_POSITION_TYPE, parentElement, errorCallback); }, toggleFileDiscussionVisibility() { - this.collapsedFileDiscussions.forEach((d) => this.toggleFileDiscussion(d)); + this.collapsedFileDiscussions.forEach((d) => this.toggleFileDiscussion({ discussion: d })); }, }, warningClasses: [ diff --git a/app/assets/javascripts/diffs/mixins/image_diff.js b/app/assets/javascripts/diffs/mixins/image_diff.js index 29be5f4330a46f..e10117fbed95b0 100644 --- a/app/assets/javascripts/diffs/mixins/image_diff.js +++ b/app/assets/javascripts/diffs/mixins/image_diff.js @@ -5,7 +5,7 @@ export default { methods: { ...mapActions(useLegacyDiffs, ['toggleFileDiscussion']), clickedToggle(discussion) { - this.toggleFileDiscussion(discussion); + this.toggleFileDiscussion({ discussion }); }, toggleText(discussion, index) { return index + 1; diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index 2b553d160d0967..ee98e130a04fc1 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -603,8 +603,8 @@ export function loadCollapsedDiff({ file, params = {} }) { * Toggles the file discussions after user clicked on the toggle discussions button. * @param {Object} discussion */ -export function toggleFileDiscussion(discussion) { - this[types.TOGGLE_FILE_DISCUSSION_EXPAND]({ discussion }); +export function toggleFileDiscussion({ discussion, expandedOnDiff }) { + this[types.TOGGLE_FILE_DISCUSSION_EXPAND]({ discussion, expandedOnDiff }); } export function toggleFileDiscussionWrappers(diff) { diff --git a/ee/app/assets/javascripts/diffs/mixins/image_diff.js b/ee/app/assets/javascripts/diffs/mixins/image_diff.js index 5c1bd649973fef..9d9d6c58ea19f6 100644 --- a/ee/app/assets/javascripts/diffs/mixins/image_diff.js +++ b/ee/app/assets/javascripts/diffs/mixins/image_diff.js @@ -10,7 +10,7 @@ export default { methods: { ...mapActions(useLegacyDiffs, ['toggleFileDiscussion']), clickedToggle(discussion) { - this.toggleFileDiscussion(discussion); + this.toggleFileDiscussion({ discussion }); }, toggleText(discussion, index) { const count = index + 1; -- GitLab From bc195d986201a9c65ad07ed072d410b4d3559f92 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Oct 2025 13:15:35 -0600 Subject: [PATCH 16/32] Handle image discussions separately --- .../notes/utils/discussion_collapse.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js index 317ba94233ccde..ab0990eb68fe78 100644 --- a/app/assets/javascripts/notes/utils/discussion_collapse.js +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -26,6 +26,8 @@ export function determineCollapseAction(discussions, resolvedDiscussionId) { const hasLineCode = isValidDiscussion && Boolean(resolvedDiscussion.line_code); const hasFileHash = isValidDiscussion && Boolean(resolvedDiscussion.diff_file?.file_hash); const isGeneralDiscussion = isValidDiscussion && !hasLineCode && !hasFileHash; + const isImagePointDiscussion = + isValidDiscussion && resolvedDiscussion.position?.position_type === 'image'; if (hasLineCode) { const lineDiscussions = discussions.filter((d) => d.line_code === resolvedDiscussion.line_code); @@ -40,10 +42,21 @@ export function determineCollapseAction(discussions, resolvedDiscussionId) { : null; } - if (!hasLineCode && hasFileHash) { + if (isImagePointDiscussion) { + collapseAction = { + type: 'file', + discussions: [resolvedDiscussion], + fileHash: resolvedDiscussion.diff_file?.file_hash, + }; + } + + if (!hasLineCode && hasFileHash && !isImagePointDiscussion) { const fileHash = resolvedDiscussion.diff_file.file_hash; const fileDiscussions = discussions.filter( - (d) => d.diff_file?.file_hash === fileHash && !d.line_code, + (d) => + d.diff_file?.file_hash === fileHash && + !d.line_code && + d.position?.position_type !== 'image', ); const allFileDiscussionsResolved = fileDiscussions.every((d) => d.resolved); -- GitLab From 68b130fbfbd4272a30cb9c9ff9abefb3558c2016 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Oct 2025 13:53:43 -0600 Subject: [PATCH 17/32] Make sure overview discussions properly collapse --- .../javascripts/notes/store/legacy_notes/actions.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 07340fe34399ba..3992e5558c7657 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -23,6 +23,7 @@ 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 { getCollapseActionFromEndpoint } from '../../utils/discussion_collapse'; import promoteTimelineEvent from '../../graphql/promote_timeline_event.mutation.graphql'; @@ -420,8 +421,14 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this[mutationType](data); this.updateResolvableDiscussionsCounts(); - this.updateMergeRequestWidget(); + + if (!isResolved && discussion) { + const collapseAction = getCollapseActionFromEndpoint(endpoint, this.discussions); + if (collapseAction) { + this.handleAutoCollapse(collapseAction); + } + } }); } -- GitLab From 58ebdf9852b0ec6d43dcce114c11dcf498c89a3a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 10:16:25 -0600 Subject: [PATCH 18/32] Individually collapse grouped discussions that appear separate --- .../javascripts/notes/components/note_body.vue | 1 + .../notes/store/legacy_notes/actions.js | 2 +- .../notes/utils/discussion_collapse.js | 18 +++++++++++++++--- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 88b5261133ba35..e60382ac5eba0f 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -195,6 +195,7 @@ export default { const collapseAction = getCollapseActionFromEndpoint( discussion.resolve_path, useNotes().discussions, + false, ); if (collapseAction) { diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 3992e5558c7657..2b8c0796fc0656 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -424,7 +424,7 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this.updateMergeRequestWidget(); if (!isResolved && discussion) { - const collapseAction = getCollapseActionFromEndpoint(endpoint, this.discussions); + const collapseAction = getCollapseActionFromEndpoint(endpoint, this.discussions, false); if (collapseAction) { this.handleAutoCollapse(collapseAction); } diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js index ab0990eb68fe78..0d1909112c4e11 100644 --- a/app/assets/javascripts/notes/utils/discussion_collapse.js +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -18,7 +18,11 @@ export function extractDiscussionIdFromEndpoint(endpoint) { * @param {string} resolvedDiscussionId - The discussion that was just resolved * @returns {Object|null} - Single collapse action or null */ -export function determineCollapseAction(discussions, resolvedDiscussionId) { +export function determineCollapseAction( + discussions, + resolvedDiscussionId, + groupDiscussions = true, +) { const resolvedDiscussion = discussions.find((d) => d.id === resolvedDiscussionId); let collapseAction = null; @@ -29,6 +33,14 @@ export function determineCollapseAction(discussions, resolvedDiscussionId) { const isImagePointDiscussion = isValidDiscussion && resolvedDiscussion.position?.position_type === 'image'; + // On Overview tab, treat all diff discussions as individual (like general discussions) + if (!groupDiscussions && (hasLineCode || hasFileHash)) { + return { + type: 'general', + discussionId: resolvedDiscussion.id, + }; + } + if (hasLineCode) { const lineDiscussions = discussions.filter((d) => d.line_code === resolvedDiscussion.line_code); const allLineDiscussionsResolved = lineDiscussions.every((d) => d.resolved); @@ -85,7 +97,7 @@ export function determineCollapseAction(discussions, resolvedDiscussionId) { * @param {Array} discussions - Array of all discussions * @returns {Object|null} - Collapse action or null */ -export function getCollapseActionFromEndpoint(endpoint, discussions) { +export function getCollapseActionFromEndpoint(endpoint, discussions, groupDiscussions = true) { const discussionId = extractDiscussionIdFromEndpoint(endpoint); - return discussionId ? determineCollapseAction(discussions, discussionId) : null; + return discussionId ? determineCollapseAction(discussions, discussionId, groupDiscussions) : null; } -- GitLab From fe11993e21dc301da1c8120f3f607839eb289ddb Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 10:31:45 -0600 Subject: [PATCH 19/32] Don't use the Notes store collapse logic when on the Changes tab --- app/assets/javascripts/diffs/stores/legacy_diffs/actions.js | 2 +- app/assets/javascripts/notes/store/legacy_notes/actions.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index ee98e130a04fc1..d34229d7062a0c 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -1282,7 +1282,7 @@ export function collapseAllFiles() { */ export function resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }) { return useNotes() - .toggleResolveNote({ endpoint, isResolved, discussion }) + .toggleResolveNote({ endpoint, isResolved, discussion, skipAutoCollapse: true }) .then((result) => { if (!isResolved && discussion) { this.handleDiffAutoCollapse({ endpoint }); diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 2b8c0796fc0656..f465a9ca89d8b8 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -411,7 +411,7 @@ export function resolveDiscussion({ discussionId }) { }); } -export function toggleResolveNote({ endpoint, isResolved, discussion }) { +export function toggleResolveNote({ endpoint, isResolved, discussion, skipAutoCollapse = false }) { const method = isResolved ? constants.UNRESOLVE_NOTE_METHOD_NAME : constants.RESOLVE_NOTE_METHOD_NAME; @@ -423,7 +423,7 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this.updateResolvableDiscussionsCounts(); this.updateMergeRequestWidget(); - if (!isResolved && discussion) { + if (!isResolved && discussion && !skipAutoCollapse) { const collapseAction = getCollapseActionFromEndpoint(endpoint, this.discussions, false); if (collapseAction) { this.handleAutoCollapse(collapseAction); -- GitLab From c4977bcf1bb0d2d952c788f0f3ce6303488961f7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 10:49:37 -0600 Subject: [PATCH 20/32] Update JSDoc blocks --- .../diffs/stores/legacy_diffs/actions.js | 29 ++++++++++++++----- .../notes/store/legacy_notes/actions.js | 8 +++++ .../notes/utils/discussion_collapse.js | 2 ++ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index d34229d7062a0c..2daaced1c0452b 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -600,8 +600,10 @@ export function loadCollapsedDiff({ file, params = {} }) { } /** - * Toggles the file discussions after user clicked on the toggle discussions button. - * @param {Object} discussion + * Toggles the expanded state of file-level discussions + * @param {Object} params - Parameters + * @param {Object} params.discussion - The discussion to toggle + * @param {boolean} params.expandedOnDiff - Optional: explicitly set expansion state */ export function toggleFileDiscussion({ discussion, expandedOnDiff }) { this[types.TOGGLE_FILE_DISCUSSION_EXPAND]({ discussion, expandedOnDiff }); @@ -1276,9 +1278,15 @@ export function collapseAllFiles() { } /** - * 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 } + * Resolves a discussion with auto-collapse behavior for the Diffs/Changes tab + * This ensures discussions collapse only when ALL related discussions on the same line/file are resolved. + * Delegates resolution to notes store, then applies grouped collapse logic via handleDiffAutoCollapse. + * + * @param {Object} params - Parameters + * @param {string} params.endpoint - The resolve/unresolve endpoint URL + * @param {boolean} params.isResolved - Current resolved state (true = currently resolved, false = currently unresolved) + * @param {boolean} params.discussion - Whether resolving entire discussion vs single note + * @returns {Promise} Resolves when discussion is resolved and collapse actions are complete */ export function resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }) { return useNotes() @@ -1292,8 +1300,15 @@ export function resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discus } /** - * Handles auto-collapse logic specific to diffs context - * @param {Object} params - { endpoint } + * Handles auto-collapse logic for resolved discussions on the Diffs/Changes tab + * Determines the appropriate collapse action based on discussion type: + * - Line discussions: Collapses all discussions on a line when all are resolved + * - File discussions: Collapses file-level discussions when all on that file are resolved + * - Image point discussions: Collapses immediately (treated as individual) + * Uses grouped collapse behavior (waits for all related discussions to be resolved). + * + * @param {Object} params - Parameters + * @param {string} params.endpoint - The resolve endpoint URL (used to extract discussion ID) */ export function handleDiffAutoCollapse({ endpoint }) { const discussionId = extractDiscussionIdFromEndpoint(endpoint); diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index f465a9ca89d8b8..a5f5976005a117 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -411,6 +411,14 @@ export function resolveDiscussion({ discussionId }) { }); } +/** + * Toggles the resolved state of a note or discussion + * @param {Object} params - Parameters + * @param {string} params.endpoint - The resolve/unresolve endpoint + * @param {boolean} params.isResolved - Current resolved state + * @param {boolean} params.discussion - Whether this is a discussion (vs single note) + * @param {boolean} params.skipAutoCollapse - Whether to skip auto-collapse (used when diffs store handles it) + */ export function toggleResolveNote({ endpoint, isResolved, discussion, skipAutoCollapse = false }) { const method = isResolved ? constants.UNRESOLVE_NOTE_METHOD_NAME diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js index 0d1909112c4e11..e00dfd3c54df93 100644 --- a/app/assets/javascripts/notes/utils/discussion_collapse.js +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -16,6 +16,7 @@ export function extractDiscussionIdFromEndpoint(endpoint) { * 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 + * @param {boolean} groupDiscussions - Whether to group related discussions (true for Diffs tab, false for Overview tab) * @returns {Object|null} - Single collapse action or null */ export function determineCollapseAction( @@ -95,6 +96,7 @@ export function determineCollapseAction( * Gets collapse action from endpoint and discussions list * @param {string} endpoint - The resolve endpoint URL * @param {Array} discussions - Array of all discussions + * @param {boolean} groupDiscussions - Whether to group related discussions (true for Diffs tab, false for Overview tab) * @returns {Object|null} - Collapse action or null */ export function getCollapseActionFromEndpoint(endpoint, discussions, groupDiscussions = true) { -- GitLab From 63b8adade93cc9ba98d6f7a6759715eed6189eee Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 11:09:57 -0600 Subject: [PATCH 21/32] Add comment about why we're checking for the state of diffs here --- app/assets/javascripts/notes/components/note_body.vue | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index e60382ac5eba0f..7d0ec3ada07a1b 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -182,6 +182,10 @@ export default { const discussion = this.getDiscussion(discussionId); if (!discussion?.resolve_path) return; + // Check if diff files are loaded to determine if we're on the diffs tab. + // While generally we prefer component-layer context via props, in this case + // note_body is deeply nested and used in many places, making prop-passing + // costly. This state check is reliable and contained to this helper. const diffsStore = useLegacyDiffs(); const diffFileLoaded = diffsStore.diffFiles.some( (f) => f.file_hash === discussion.diff_file?.file_hash, -- GitLab From a6c57cc9ef5da03ab9f181f36abca711200ff5f0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 12:45:23 -0600 Subject: [PATCH 22/32] Update tests with correct `discussions` property wrapper --- .../diffs/components/diff_discussions_spec.js | 4 +++- .../diffs/components/image_diff_overlay_spec.js | 14 ++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/spec/frontend/diffs/components/diff_discussions_spec.js b/spec/frontend/diffs/components/diff_discussions_spec.js index 89170bbb42743c..f97f87a2f2a843 100644 --- a/spec/frontend/diffs/components/diff_discussions_spec.js +++ b/spec/frontend/diffs/components/diff_discussions_spec.js @@ -72,7 +72,9 @@ describe('DiffDiscussions', () => { findDiffNotesToggle().trigger('click'); - expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith(discussions[0]); + expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith({ + discussion: discussions[0], + }); }); it('renders expand button when discussion is collapsed', () => { diff --git a/spec/frontend/diffs/components/image_diff_overlay_spec.js b/spec/frontend/diffs/components/image_diff_overlay_spec.js index 50b9ab6b56c00d..ed8841e7c7f227 100644 --- a/spec/frontend/diffs/components/image_diff_overlay_spec.js +++ b/spec/frontend/diffs/components/image_diff_overlay_spec.js @@ -115,12 +115,14 @@ describe('Diffs image diff overlay component', () => { getAllImageBadges().at(0).vm.$emit('click'); expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith({ - id: '1', - position: { - height: 200, - width: 100, - x: 10, - y: 10, + discussion: { + id: '1', + position: { + height: 200, + width: 100, + x: 10, + y: 10, + }, }, }); }); -- GitLab From 0eb3c367525dda3967552c67d12f902e61871bce Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 13:58:46 -0600 Subject: [PATCH 23/32] Test new and updated features of the collapse helpers --- .../notes/utils/discussion_collapse_spec.js | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/spec/frontend/notes/utils/discussion_collapse_spec.js b/spec/frontend/notes/utils/discussion_collapse_spec.js index 5827b3dd2a8345..5cac70c750c43a 100644 --- a/spec/frontend/notes/utils/discussion_collapse_spec.js +++ b/spec/frontend/notes/utils/discussion_collapse_spec.js @@ -1,6 +1,7 @@ import { extractDiscussionIdFromEndpoint, determineCollapseAction, + getCollapseActionFromEndpoint, } from '~/notes/utils/discussion_collapse'; function createDiscussion(properties = {}) { @@ -144,6 +145,86 @@ describe('Discussion Collapse Utilities', () => { }); }); + describe('image point discussions', () => { + function createImagePointDiscussion({ id, resolved = true, fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: { file_hash: fileHash }, + position: { position_type: 'image' }, + }); + } + + it('collapses image point discussions immediately as individual file actions', () => { + const discussions = [ + createImagePointDiscussion({ id: 'discussion1', resolved: true }), + createFileDiscussion({ id: 'discussion2', resolved: false }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + + it('excludes image point discussions from file-level discussion grouping', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1', resolved: true }), + createImagePointDiscussion({ id: 'discussion2', resolved: false }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + }); + + describe('groupDiscussions parameter', () => { + it.each` + groupDiscussions | discussionType | hasOtherUnresolved | expectedResult | description + ${false} | ${'line'} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'returns general action for line discussions on Overview tab regardless of other discussions'} + ${false} | ${'file'} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'returns general action for file discussions on Overview tab regardless of other discussions'} + ${true} | ${'line'} | ${true} | ${null} | ${'returns null for line discussions on Diffs tab when others unresolved'} + ${undefined} | ${'line'} | ${false} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${'uses grouped behavior by default (groupDiscussions defaults to true)'} + `( + '$description', + ({ groupDiscussions, discussionType, hasOtherUnresolved, expectedResult }) => { + const discussions = + discussionType === 'line' + ? [ + createLineDiscussion({ id: 'discussion1', resolved: true, lineCode: 'abc_1_1' }), + ...(hasOtherUnresolved + ? [ + createLineDiscussion({ + id: 'discussion2', + resolved: false, + lineCode: 'abc_1_1', + }), + ] + : []), + ] + : [ + createFileDiscussion({ id: 'discussion1', resolved: true }), + ...(hasOtherUnresolved + ? [createFileDiscussion({ id: 'discussion2', resolved: false })] + : []), + ]; + + const result = determineCollapseAction(discussions, 'discussion1', groupDiscussions); + + expect(result).toEqual(expectedResult); + }, + ); + }); + describe('edge cases', () => { it('returns null when discussion is not found', () => { const discussions = [createLineDiscussion({ id: 'discussion1' })]; @@ -208,4 +289,21 @@ describe('Discussion Collapse Utilities', () => { }); }); }); + + describe('getCollapseActionFromEndpoint', () => { + it.each` + endpoint | groupDiscussions | discussionExists | expectedResult | description + ${'/discussions/discussion1/resolve'} | ${undefined} | ${true} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${'extracts ID and determines collapse action with default grouping'} + ${'/discussions/discussion1/resolve'} | ${false} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'passes groupDiscussions parameter through'} + ${'/invalid/endpoint'} | ${undefined} | ${true} | ${null} | ${'returns null for invalid endpoint'} + ${'/discussions/nonexistent/resolve'} | ${undefined} | ${false} | ${null} | ${'returns null when discussion not found'} + `('$description', ({ endpoint, groupDiscussions, discussionExists, expectedResult }) => { + const discussions = discussionExists + ? [createLineDiscussion({ id: 'discussion1' })] + : [createLineDiscussion({ id: 'other' })]; + + const result = getCollapseActionFromEndpoint(endpoint, discussions, groupDiscussions); + expect(result).toEqual(expectedResult); + }); + }); }); -- GitLab From 8e33b8e245b730133c34b8ac0aa05872fce1699e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 13:59:22 -0600 Subject: [PATCH 24/32] Test new auto-collapse features of applying suggestions --- .../notes/components/note_body_spec.js | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) diff --git a/spec/frontend/notes/components/note_body_spec.js b/spec/frontend/notes/components/note_body_spec.js index b6f90b3a5e0bd4..bf46ec431bbd27 100644 --- a/spec/frontend/notes/components/note_body_spec.js +++ b/spec/frontend/notes/components/note_body_spec.js @@ -139,6 +139,159 @@ describe('issue_note_body component', () => { }); }); + describe('applying suggestions that trigger thread auto-collapse', () => { + let diffsStore; + let notesStore; + let handleDiffAutoCollapse; + let handleAutoCollapse; + let submitSuggestion; + let submitSuggestionBatch; + let triggerAutoCollapse; + + const createDiffNote = () => ({ + ...note, + type: 'DiffNote', + discussion_id: 'discussion1', + suggestions: [{ id: 'suggestion1' }], + }); + + const setupDiscussion = (resolvePathExists = true) => { + const discussion = { + id: 'discussion1', + resolve_path: resolvePathExists ? '/discussions/discussion1/resolve' : null, + diff_file: { file_hash: 'file123' }, + }; + notesStore.discussions = [discussion]; + notesStore.getDiscussion = jest.fn().mockReturnValue(discussion); + return discussion; + }; + + beforeEach(() => { + diffsStore = useLegacyDiffs(); + notesStore = useNotes(); + + handleDiffAutoCollapse = jest.fn(); + handleAutoCollapse = jest.fn(); + submitSuggestion = jest.fn().mockResolvedValue({}); + submitSuggestionBatch = jest.fn().mockResolvedValue({}); + + diffsStore.handleDiffAutoCollapse = handleDiffAutoCollapse; + notesStore.handleAutoCollapse = handleAutoCollapse; + notesStore.submitSuggestion = submitSuggestion; + notesStore.submitSuggestionBatch = submitSuggestionBatch; + + setupDiscussion(); + }); + + describe('triggerAutoCollapse', () => { + beforeEach(() => { + createComponent({ note: createDiffNote() }); + triggerAutoCollapse = wrapper.vm.triggerAutoCollapse; + }); + + it.each` + diffFilesLoaded | expectedStore | description + ${true} | ${'diffs'} | ${'calls diffs store handleDiffAutoCollapse when diff files are loaded'} + ${false} | ${'notes'} | ${'calls notes store handleAutoCollapse when diff files are not loaded'} + `('$description', ({ diffFilesLoaded, expectedStore }) => { + diffsStore.diffFiles = diffFilesLoaded ? [{ file_hash: 'file123' }] : []; + + triggerAutoCollapse('discussion1'); + + if (expectedStore === 'diffs') { + expect(handleDiffAutoCollapse).toHaveBeenCalledWith({ + endpoint: '/discussions/discussion1/resolve', + }); + expect(handleAutoCollapse).not.toHaveBeenCalled(); + } else { + expect(handleAutoCollapse).toHaveBeenCalled(); + expect(handleDiffAutoCollapse).not.toHaveBeenCalled(); + } + }); + + it('does nothing when discussion has no resolve_path', () => { + setupDiscussion(false); + createComponent({ note: createDiffNote() }); + + wrapper.vm.triggerAutoCollapse('discussion1'); + + expect(handleDiffAutoCollapse).not.toHaveBeenCalled(); + expect(handleAutoCollapse).not.toHaveBeenCalled(); + }); + }); + + describe('applySuggestion', () => { + it('calls triggerAutoCollapse for DiffNote after successful suggestion application', async () => { + const diffNote = createDiffNote(); + createComponent({ note: diffNote }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestion({ + suggestionId: 'suggestion1', + flashContainer: null, + callback: jest.fn(), + message: '', + }); + + expect(submitSuggestion).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledWith('discussion1'); + }); + + it('does not call triggerAutoCollapse for non-DiffNote', async () => { + const regularNote = { + ...note, + type: 'DiscussionNote', + suggestions: [{ id: 'suggestion1' }], + }; + createComponent({ note: regularNote }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestion({ + suggestionId: 'suggestion1', + flashContainer: null, + callback: jest.fn(), + message: '', + }); + + expect(submitSuggestion).toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); + }); + }); + + describe('applySuggestionBatch', () => { + beforeEach(() => { + notesStore.batchSuggestionsInfo = [ + { discussionId: 'discussion1' }, + { discussionId: 'discussion2' }, + ]; + }); + + it('calls triggerAutoCollapse for each discussion in batch for DiffNote', async () => { + const diffNote = createDiffNote(); + createComponent({ note: diffNote }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); + + expect(submitSuggestionBatch).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledTimes(2); + expect(spy).toHaveBeenCalledWith('discussion1'); + expect(spy).toHaveBeenCalledWith('discussion2'); + }); + + it('does not call triggerAutoCollapse for non-DiffNote', async () => { + const regularNote = { ...note, type: 'DiscussionNote' }; + createComponent({ note: regularNote }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); + + expect(submitSuggestionBatch).toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); + }); + }); + }); + describe('duo code review feedback', () => { it.each` userType | type | exists | existsText -- GitLab From 6842658cb3e46202930c20dcf9ac1a8afde8fe45 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 14:07:59 -0600 Subject: [PATCH 25/32] Test new diffs actions for auto-collapsing resolved threads --- .../diffs/stores/legacy_diffs/actions_spec.js | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index 542bce7334a476..c4f0b0a9ed075e 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -2786,4 +2786,72 @@ describe('legacyDiffs actions', () => { return testAction(store.setCurrentFileHash, 'note_filehash', {}, [], []); }); }); + + describe('resolveDiscussionWithAutoCollapse', () => { + let notesStore; + + beforeEach(() => { + notesStore = useNotes(); + notesStore.toggleResolveNote = jest.fn().mockResolvedValue({}); + store.handleDiffAutoCollapse = jest.fn(); + }); + + it.each` + isResolved | discussion | expectedCalls | description + ${false} | ${true} | ${1} | ${'calls handleDiffAutoCollapse when resolving a discussion'} + ${true} | ${true} | ${0} | ${'does not call handleDiffAutoCollapse when unresolving'} + ${false} | ${false} | ${0} | ${'does not call handleDiffAutoCollapse when resolving a single note'} + `('$description', async ({ isResolved, discussion, expectedCalls }) => { + const endpoint = '/discussions/discussion1/resolve'; + + await store.resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }); + + expect(notesStore.toggleResolveNote).toHaveBeenCalledWith({ + endpoint, + isResolved, + discussion, + skipAutoCollapse: true, + }); + expect(store.handleDiffAutoCollapse).toHaveBeenCalledTimes(expectedCalls); + }); + }); + + describe('handleDiffAutoCollapse', () => { + let notesStore; + + beforeEach(() => { + notesStore = useNotes(); + store.toggleLineDiscussions = jest.fn(); + store.toggleFileDiscussion = jest.fn(); + }); + + it.each` + discussionType | setupDiscussions | expectedAction | expectedCall + ${'line'} | ${[{ id: 'disc1', resolved: true, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }]} | ${'toggleLineDiscussions'} | ${{ lineCode: 'abc_1_1', fileHash: 'file123', expanded: false }} + ${'file'} | ${[{ id: 'disc1', resolved: true, line_code: null, diff_file: { file_hash: 'file123' } }]} | ${'toggleFileDiscussion'} | ${{ discussion: expect.any(Object), expandedOnDiff: false }} + ${'image'} | ${[{ id: 'disc1', resolved: true, line_code: null, diff_file: { file_hash: 'file123' }, position: { position_type: 'image' } }]} | ${'toggleFileDiscussion'} | ${{ discussion: expect.any(Object), expandedOnDiff: false }} + `( + 'handles $discussionType discussions correctly', + async ({ setupDiscussions, expectedAction, expectedCall }) => { + notesStore.discussions = setupDiscussions; + + await store.handleDiffAutoCollapse({ endpoint: '/discussions/disc1/resolve' }); + + expect(store[expectedAction]).toHaveBeenCalledWith(expectedCall); + }, + ); + + it.each` + endpoint | discussions | description + ${'/invalid'} | ${[]} | ${'does nothing when endpoint is invalid'} + ${'/discussions/disc1/resolve'} | ${[{ id: 'disc1', resolved: true, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }, { id: 'disc2', resolved: false, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }]} | ${'does nothing when not all line discussions are resolved'} + `('$description', async ({ endpoint, discussions }) => { + notesStore.discussions = discussions; + + await store.handleDiffAutoCollapse({ endpoint }); + + expect(store.toggleLineDiscussions).not.toHaveBeenCalled(); + expect(store.toggleFileDiscussion).not.toHaveBeenCalled(); + }); + }); }); -- GitLab From c96dc7849232232318662e79e0e976d2cb9a8546 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 14:12:54 -0600 Subject: [PATCH 26/32] Test new notes action behavior for auto-collapsing resolved threads --- .../notes/store/legacy_notes/actions_spec.js | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index cce19c35841d04..72ec1996c5a066 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -640,6 +640,36 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('with auto-collapse', () => { + const endpoint = `${TEST_HOST}/discussions/discussion1/resolve`; + const result = { resolved: true, expanded: true, discussion_id: 'discussion1', id: 1 }; + + beforeEach(() => { + axiosMock.onPut(endpoint).reply(HTTP_STATUS_OK, result); + store.handleAutoCollapse = jest.fn(); + store.discussions = [ + { + id: 'discussion1', + resolved: true, + resolve_path: endpoint, + diff_file: { file_hash: 'file123' }, + }, + ]; + }); + + it.each` + isResolved | skipAutoCollapse | expectedCalls | description + ${false} | ${undefined} | ${1} | ${'calls handleAutoCollapse when resolving discussion with default skipAutoCollapse'} + ${false} | ${false} | ${1} | ${'calls handleAutoCollapse when resolving discussion with skipAutoCollapse false'} + ${false} | ${true} | ${0} | ${'does not call handleAutoCollapse when skipAutoCollapse is true'} + ${true} | ${false} | ${0} | ${'does not call handleAutoCollapse when unresolving'} + `('$description', async ({ isResolved, skipAutoCollapse, expectedCalls }) => { + await store.toggleResolveNote({ endpoint, isResolved, discussion: true, skipAutoCollapse }); + + expect(store.handleAutoCollapse).toHaveBeenCalledTimes(expectedCalls); + }); + }); }); describe('handleAutoCollapse', () => { -- GitLab From 2123ecf8506dec1ed28f8261d4b6ef4b1f596e72 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 28 Oct 2025 17:41:10 -0600 Subject: [PATCH 27/32] Fix some test issues --- .../notes/components/note_body_spec.js | 49 ++++++++----------- .../notes/store/legacy_notes/actions_spec.js | 2 +- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/spec/frontend/notes/components/note_body_spec.js b/spec/frontend/notes/components/note_body_spec.js index bf46ec431bbd27..596a5453f7facc 100644 --- a/spec/frontend/notes/components/note_body_spec.js +++ b/spec/frontend/notes/components/note_body_spec.js @@ -41,6 +41,7 @@ describe('issue_note_body component', () => { useLegacyDiffs(); useNotes().setNoteableData(noteableDataMock); useNotes().setNotesData(notesDataMock); + useNotes().discussions = []; useMrNotes(); createComponent(); }); @@ -146,7 +147,6 @@ describe('issue_note_body component', () => { let handleAutoCollapse; let submitSuggestion; let submitSuggestionBatch; - let triggerAutoCollapse; const createDiffNote = () => ({ ...note, @@ -158,11 +158,13 @@ describe('issue_note_body component', () => { const setupDiscussion = (resolvePathExists = true) => { const discussion = { id: 'discussion1', + resolved: true, resolve_path: resolvePathExists ? '/discussions/discussion1/resolve' : null, diff_file: { file_hash: 'file123' }, + notes: [], }; notesStore.discussions = [discussion]; - notesStore.getDiscussion = jest.fn().mockReturnValue(discussion); + return discussion; }; @@ -184,34 +186,25 @@ describe('issue_note_body component', () => { }); describe('triggerAutoCollapse', () => { - beforeEach(() => { - createComponent({ note: createDiffNote() }); - triggerAutoCollapse = wrapper.vm.triggerAutoCollapse; - }); - it.each` - diffFilesLoaded | expectedStore | description - ${true} | ${'diffs'} | ${'calls diffs store handleDiffAutoCollapse when diff files are loaded'} - ${false} | ${'notes'} | ${'calls notes store handleAutoCollapse when diff files are not loaded'} - `('$description', ({ diffFilesLoaded, expectedStore }) => { + diffFilesLoaded | expectedDiffsCalls | expectedNotesCalls | description + ${true} | ${1} | ${0} | ${'calls diffs store handleDiffAutoCollapse when diff files are loaded'} + ${false} | ${0} | ${1} | ${'calls notes store handleAutoCollapse when diff files are not loaded'} + `('$description', ({ diffFilesLoaded, expectedDiffsCalls, expectedNotesCalls }) => { diffsStore.diffFiles = diffFilesLoaded ? [{ file_hash: 'file123' }] : []; - triggerAutoCollapse('discussion1'); - - if (expectedStore === 'diffs') { - expect(handleDiffAutoCollapse).toHaveBeenCalledWith({ - endpoint: '/discussions/discussion1/resolve', - }); - expect(handleAutoCollapse).not.toHaveBeenCalled(); - } else { - expect(handleAutoCollapse).toHaveBeenCalled(); - expect(handleDiffAutoCollapse).not.toHaveBeenCalled(); - } + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); + + wrapper.vm.triggerAutoCollapse('discussion1'); + + expect(handleDiffAutoCollapse).toHaveBeenCalledTimes(expectedDiffsCalls); + expect(handleAutoCollapse).toHaveBeenCalledTimes(expectedNotesCalls); }); it('does nothing when discussion has no resolve_path', () => { setupDiscussion(false); - createComponent({ note: createDiffNote() }); + + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); wrapper.vm.triggerAutoCollapse('discussion1'); @@ -222,8 +215,7 @@ describe('issue_note_body component', () => { describe('applySuggestion', () => { it('calls triggerAutoCollapse for DiffNote after successful suggestion application', async () => { - const diffNote = createDiffNote(); - createComponent({ note: diffNote }); + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); await wrapper.vm.applySuggestion({ @@ -243,7 +235,7 @@ describe('issue_note_body component', () => { type: 'DiscussionNote', suggestions: [{ id: 'suggestion1' }], }; - createComponent({ note: regularNote }); + createComponent({ note: regularNote, file: { file_path: 'test.js' } }); const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); await wrapper.vm.applySuggestion({ @@ -267,8 +259,7 @@ describe('issue_note_body component', () => { }); it('calls triggerAutoCollapse for each discussion in batch for DiffNote', async () => { - const diffNote = createDiffNote(); - createComponent({ note: diffNote }); + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); @@ -281,7 +272,7 @@ describe('issue_note_body component', () => { it('does not call triggerAutoCollapse for non-DiffNote', async () => { const regularNote = { ...note, type: 'DiscussionNote' }; - createComponent({ note: regularNote }); + createComponent({ note: regularNote, file: { file_path: 'test.js' } }); const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index 72ec1996c5a066..f35624f04dc679 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -650,7 +650,7 @@ describe('Actions Notes Store', () => { store.handleAutoCollapse = jest.fn(); store.discussions = [ { - id: 'discussion1', + id: 1, resolved: true, resolve_path: endpoint, diff_file: { file_hash: 'file123' }, -- GitLab From 8036ad7f334dcfd01baf606b456c2773ad2c25d9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 29 Oct 2025 00:34:10 -0600 Subject: [PATCH 28/32] Ensure the ID from the endpoint is a Number if it's numeric --- .../javascripts/notes/utils/discussion_collapse.js | 7 ++++++- spec/frontend/notes/store/legacy_notes/actions_spec.js | 10 ++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js index e00dfd3c54df93..d82f9434a3fcfb 100644 --- a/app/assets/javascripts/notes/utils/discussion_collapse.js +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -9,7 +9,12 @@ export function extractDiscussionIdFromEndpoint(endpoint) { } const match = endpoint.match(/discussions\/([^/]+)\/resolve$/); - return match ? match[1] : null; + if (!match) return null; + + const id = match[1]; + const numericId = Number(id); + + return Number.isNaN(numericId) ? id : numericId; } /** diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index f35624f04dc679..501765df0a460c 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -642,8 +642,8 @@ describe('Actions Notes Store', () => { }); describe('with auto-collapse', () => { - const endpoint = `${TEST_HOST}/discussions/discussion1/resolve`; - const result = { resolved: true, expanded: true, discussion_id: 'discussion1', id: 1 }; + const endpoint = `${TEST_HOST}/discussions/1/resolve`; + const result = { resolved: true, expanded: true, id: 1 }; beforeEach(() => { axiosMock.onPut(endpoint).reply(HTTP_STATUS_OK, result); @@ -651,17 +651,15 @@ describe('Actions Notes Store', () => { store.discussions = [ { id: 1, - resolved: true, + resolved: false, resolve_path: endpoint, - diff_file: { file_hash: 'file123' }, }, ]; }); it.each` isResolved | skipAutoCollapse | expectedCalls | description - ${false} | ${undefined} | ${1} | ${'calls handleAutoCollapse when resolving discussion with default skipAutoCollapse'} - ${false} | ${false} | ${1} | ${'calls handleAutoCollapse when resolving discussion with skipAutoCollapse false'} + ${false} | ${false} | ${1} | ${'calls handleAutoCollapse when resolving discussion'} ${false} | ${true} | ${0} | ${'does not call handleAutoCollapse when skipAutoCollapse is true'} ${true} | ${false} | ${0} | ${'does not call handleAutoCollapse when unresolving'} `('$description', async ({ isResolved, skipAutoCollapse, expectedCalls }) => { -- GitLab From 2a1ea31672703cdb4a7d4d58ebd77a4fc378307f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 29 Oct 2025 02:11:25 -0600 Subject: [PATCH 29/32] Fix integration tests for all collapsing discussions --- ...single_discussion_in_merge_request_spec.rb | 7 ++- ...diff_notes_and_discussions_resolve_spec.rb | 56 ++++++++++++------- .../helpers/merge_request_diff_helpers.rb | 13 +++++ 3 files changed, 55 insertions(+), 21 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 fdd5c9ead73c56..b2b92eb8417498 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,10 +45,13 @@ 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_all_collapsed_discussions + end + + 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)) - 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)) + click_button '1 reply' + within_testid('reply-wrapper') do click_button 'Reopen thread' 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)) 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 8c9c7440db7dfa..3df142037daaab 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,10 +46,11 @@ it 'allows user to mark a note as resolved' do page.within '.diff-content .note' do find_by_testid('resolve-line-button').click + end - wait_for_requests - expand_all_collapsed_discussions + expand_auto_collapsed_replies + page.within '.diff-content .note' do expect(find_by_testid('resolve-line-button')['aria-label']).to eq("Resolved by #{user.name}") end @@ -77,7 +78,11 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click - expand_all_collapsed_discussions + end + + expand_auto_collapsed_replies + + page.within '.diff-content' do click_button 'Reopen thread' end @@ -129,7 +134,7 @@ end it 'shows resolved thread when toggled', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/327439' do - find('.diff-comment-avatar-holders').click + expand_auto_collapsed_replies expect(find('.diffs .diff-file .notes_holder')).to be_visible end @@ -233,12 +238,13 @@ it 'updates updated text after resolving note' do page.within '.diff-content .note' do - resolve_button = find_by_testid('resolve-line-button') + find_by_testid('resolve-line-button').click + end - resolve_button.click - wait_for_requests - expand_all_collapsed_discussions + expand_auto_collapsed_replies + page.within '.diff-content .note' do + resolve_button = find_by_testid('resolve-line-button') expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end @@ -259,10 +265,11 @@ it 'marks thread as resolved when resolving single note' do page.within("#note_#{note.id}") do first('[data-testid="resolve-line-button"]').click + end - wait_for_requests - expand_all_collapsed_discussions + expand_auto_collapsed_replies + page.within("#note_#{note.id}") do expect(first('[data-testid="resolve-line-button"]')['aria-label']).to eq("Resolved by #{user.name}") end @@ -272,15 +279,13 @@ end it 'resolves thread' do - resolve_buttons = page.all('.note [data-testid="resolve-line-button"]', count: 1) - resolve_buttons.each do |button| + page.all('.note [data-testid="resolve-line-button"]', count: 1).each do |button| button.click end - wait_for_requests - expand_all_collapsed_discussions + expand_auto_collapsed_replies - resolve_buttons.each do |button| + page.all('.note [data-testid="resolve-line-button"]').each do |button| expect(button['aria-label']).to eq("Resolved by #{user.name}") end @@ -346,11 +351,13 @@ it 'updates updated text after resolving note' do page.within('.diff-content .note', match: :first) do - resolve_button = find_by_testid('resolve-line-button') + find_by_testid('resolve-line-button').click + end - resolve_button.click - wait_for_requests + expand_auto_collapsed_replies + page.within('.diff-content .note', match: :first) do + resolve_button = find_by_testid('resolve-line-button') expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end @@ -392,6 +399,8 @@ find_by_testid('resolve-line-button').click end + expand_auto_collapsed_replies + page.within '.diff-content' do expect(page).to have_selector('.btn', text: 'Reopen thread') end @@ -414,6 +423,11 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + end + + expand_auto_collapsed_replies + + page.within '.diff-content' do click_button 'Reopen thread' end @@ -441,9 +455,11 @@ it 'allows user to comment & unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + end - wait_for_requests + expand_auto_collapsed_replies + page.within '.diff-content' do find_field('Reply…').click find('.js-unresolve-checkbox') @@ -499,6 +515,8 @@ find_by_testid('resolve-line-button').click end + expand_auto_collapsed_replies + page.within '.diff-content' do expect(page).to have_selector('.btn', text: 'Reopen thread') end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index 199575b15daf49..6877bf325807fc 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -166,4 +166,17 @@ def expand_all_collapsed_discussions end end end + + def expand_auto_collapsed_replies + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('[data-testid="expand-replies-button"]', wait: 1) + begin + first('[data-testid="expand-replies-button"]', wait: 1).click + wait_for_requests + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end + end + end end -- GitLab From d7a8a3bba141d682a5efbc8f583da2077fc6d35c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 29 Oct 2025 18:50:26 -0600 Subject: [PATCH 30/32] Fix QA helper that isn't expanding all types of collapsed threads --- qa/qa/page/merge_request/show.rb | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 02664fee928f98..9d618480ffaa53 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -556,9 +556,23 @@ def has_exposed_artifact_with_name?(name) end def expand_collapsed_discussions - all_elements('discussion-avatar', minimum: 1) - .reject { |el| el[:class].include?('diff-notes-collapse') } - .each(&:click) + loop do + expand_button = all_elements('expand-replies-button', minimum: 0).first + break unless expand_button + + expand_button.click + wait_for_requests + end + + loop do + avatar = all_elements('discussion-avatar', minimum: 0) + .reject { |el| el[:class].include?('diff-notes-collapse') } + .first + break unless avatar + + avatar.click + wait_for_requests + end end private -- GitLab From 49cac227c18b7fb6aaabddbcbf43af69f0700411 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 29 Oct 2025 22:23:40 -0600 Subject: [PATCH 31/32] Ensure the UI is updated before asserting on the content --- .../user_resolves_diff_notes_and_discussions_resolve_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 3df142037daaab..e2dbe552643144 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 @@ -283,10 +283,11 @@ button.click end + wait_for_requests expand_auto_collapsed_replies - page.all('.note [data-testid="resolve-line-button"]').each do |button| - expect(button['aria-label']).to eq("Resolved by #{user.name}") + page.all('.note [data-testid="resolve-line-button"]').each do + expect(page).to have_css("button[aria-label='Resolved by #{user.name}']") end page.within(first('.discussions-counter')) do -- GitLab From 607bfb589a1dfe97c5207c385abbd96571d58d6d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 30 Oct 2025 15:19:54 -0600 Subject: [PATCH 32/32] Implement backend review notes --- qa/qa/page/merge_request/show.rb | 4 ++-- ...e_for_single_discussion_in_merge_request_spec.rb | 2 -- spec/support/helpers/features/notes_helpers.rb | 13 ------------- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 9d618480ffaa53..49db769139f5c3 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -566,8 +566,8 @@ def expand_collapsed_discussions loop do avatar = all_elements('discussion-avatar', minimum: 0) - .reject { |el| el[:class].include?('diff-notes-collapse') } - .first + .find { |el| el[:class].exclude?('diff-notes-collapse') } + break unless avatar avatar.click 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 b2b92eb8417498..289744e5bdc649 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,8 +3,6 @@ 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/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index ceceba4a054e89..55c1a29bef84d9 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -53,18 +53,5 @@ def preview_note(text, issuable_type = nil) yield if block_given? end end - - def expand_all_collapsed_discussions - allowing_for_delay(interval: 0.5, retries: 3) do - while has_selector?('.js-diff-comment-avatar:not(.diff-notes-collapse)', wait: 1) - begin - first('.js-diff-comment-avatar:not(.diff-notes-collapse)', wait: 1, minimum: 1).click - wait_for_requests - rescue Selenium::WebDriver::Error::StaleElementReferenceError - raise Capybara::ExpectationNotMet - end - end - end - end end end -- GitLab