From b6ab76b225017a1e5046a11bc4456a918b76a66c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 4 Sep 2025 21:52:18 -0600 Subject: [PATCH] Backport comment suggestion line range fix to 18-3 This commit is the cherrypick of 3 commits from `master`. The chain of MRs ends with this one (all are linked there): https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202711 - 37127e786e9f469e8a98d6302fb6049acad01122 - 277401e1431632623c7979fb74a9941ad127f0cd - f33ce13c1806defdcc4a278fbb0327f412dfd8ad Changelog: fixed --- .../diffs/stores/legacy_diffs/actions.js | 43 ++++++ .../notes/components/discussion_notes.vue | 1 + .../notes/components/note_body.vue | 6 + .../notes/components/noteable_discussion.vue | 6 + .../notes/components/noteable_note.vue | 5 + .../diffs/stores/legacy_diffs/actions_spec.js | 133 ++++++++++++++++++ .../notes/components/note_body_spec.js | 17 +++ .../components/noteable_discussion_spec.js | 76 +++++++++- .../notes/components/noteable_note_spec.js | 56 +++++++- 9 files changed, 332 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index 14db30db975141..17e6f378fa2532 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -59,6 +59,7 @@ import { EVT_MR_PREPARED, FILE_DIFF_POSITION_TYPE, EVT_DISCUSSIONS_ASSIGNED, + OLD_LINE_TYPE, } from '../../constants'; import { DISCUSSION_SINGLE_DIFF_FAILED, @@ -662,6 +663,48 @@ export async function saveDiffDiscussion({ note, formData }) { }); } +export function getLinesForDiscussion({ discussion }) { + const lines = []; + + if (!discussion?.diff_file || !discussion?.position) { + return lines; + } + + const diffFile = this.diffFiles.find((file) => file.file_hash === discussion.diff_file.file_hash); + + if (!diffFile) { + return lines; + } + + const diffLines = diffFile[INLINE_DIFF_LINES_KEY]; + let isAdding = false; + const { start, end } = discussion.position?.line_range || {}; + + if (!start || !end) { + return lines; + } + + for (let i = 0, diffLinesLength = diffLines.length - 1; i <= diffLinesLength; i += 1) { + const line = diffLines[i]; + + if (start.line_code === line.line_code) { + isAdding = true; + } + + if (isAdding) { + if (line.type !== OLD_LINE_TYPE) { + lines.push(line); + } + + if (end.line_code === line.line_code) { + break; + } + } + } + + return lines; +} + export function toggleTreeOpen(path) { this[types.TOGGLE_FOLDER_OPEN](path); } diff --git a/app/assets/javascripts/notes/components/discussion_notes.vue b/app/assets/javascripts/notes/components/discussion_notes.vue index 78953a62733011..f626d689b3d7cb 100644 --- a/app/assets/javascripts/notes/components/discussion_notes.vue +++ b/app/assets/javascripts/notes/components/discussion_notes.vue @@ -137,6 +137,7 @@ export default { :is="componentName(firstNote)" :note="componentData(firstNote)" :line="line || diffLine" + :discussion="discussion" :discussion-file="discussion.diff_file" :commit="commit" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 1bf4fec6cebb87..100bd2fcc20685 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -36,6 +36,11 @@ export default { required: false, default: null, }, + lines: { + type: Array, + required: false, + default: () => [], + }, file: { type: Object, required: false, @@ -234,6 +239,7 @@ export default { :note-body="noteBody" :note-id="note.id" :line="line" + :lines="lines" :note="note" :diff-file="file" :save-button-title="saveButtonTitle" diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 8d65585f760a69..5063f0c7d00899 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -14,6 +14,7 @@ import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link import { detectAndConfirmSensitiveTokens } from '~/lib/utils/secret_detection'; import { FILE_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants'; import { useNotes } from '~/notes/store/legacy_notes'; +import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import eventHub from '../event_hub'; import noteable from '../mixins/noteable'; import resolvable from '../mixins/resolvable'; @@ -188,6 +189,9 @@ export default { isDiscussionInternal() { return this.discussion.notes[0]?.internal; }, + commentLines() { + return this.getLinesForDiscussion({ discussion: this.discussion }); + }, discussionHolderClass() { return { 'is-replying': this.isReplying, @@ -216,6 +220,7 @@ export default { 'removeConvertedDiscussion', 'expandDiscussion', ]), + ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), showReplyForm(text) { this.isReplying = true; @@ -378,6 +383,7 @@ export default { :discussion="discussion" :diff-file="diffFile" :line="diffLine" + :lines="commentLines" :save-button-title="saveButtonTitle" :autofocus="!hasDraft" :autosave-key="autosaveKey" diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index 367e3fbc743b40..da7ff174b67035 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -182,6 +182,9 @@ export default { } return ''; }, + commentLines() { + return this.getLinesForDiscussion({ discussion: this.discussion }); + }, actionText() { if (!this.commit) { return ''; @@ -281,6 +284,7 @@ export default { 'updateAssignees', 'setSelectedCommentPositionHover', ]), + ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), editHandler() { this.isEditing = true; this.setSelectedCommentPositionHover(); @@ -539,6 +543,7 @@ export default { :note="note" :can-edit="canEdit" :line="line" + :lines="commentLines" :file="diffFile" :is-editing="isEditing" :autosave-key="autosaveKey" diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index 57f6fe18ea37f3..22580b98e7c3e3 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -9,9 +9,11 @@ import { getDiffFileMock } from 'jest/diffs/mock_data/diff_file'; import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE, + INLINE_DIFF_LINES_KEY, PARALLEL_DIFF_VIEW_TYPE, EVT_MR_PREPARED, FILE_DIFF_POSITION_TYPE, + OLD_LINE_TYPE, } from '~/diffs/constants'; import { BUILDING_YOUR_MR, @@ -1321,6 +1323,137 @@ describe('legacyDiffs actions', () => { }); }); + describe('getLinesForDiscussion', () => { + it('returns empty array when discussion is null or undefined', () => { + expect(store.getLinesForDiscussion({ discussion: null })).toEqual([]); + expect(store.getLinesForDiscussion({ discussion: undefined })).toEqual([]); + }); + + it('returns empty array when discussion has no position', () => { + const result = store.getLinesForDiscussion({ discussion: {} }); + expect(result).toEqual([]); + }); + + it('returns empty array when line_range is missing', () => { + const discussion = { position: {} }; + const result = store.getLinesForDiscussion({ discussion }); + expect(result).toEqual([]); + }); + + it('returns empty array when diffFile is not found', () => { + const discussion = { + diff_file: { file_hash: 'nonexistent_hash' }, + position: { + line_range: { + start: { line_code: 'start_line' }, + end: { line_code: 'end_line' }, + }, + }, + }; + + store.diffFiles = [{ file_hash: 'different_hash' }]; + + const result = store.getLinesForDiscussion({ discussion }); + expect(result).toEqual([]); + }); + + it('returns correct lines for a valid discussion', () => { + const diffLines = [ + { line_code: 'line1', type: 'new' }, + { line_code: 'line2', type: 'new' }, + { line_code: 'line3', type: 'new' }, + { line_code: 'line4', type: 'new' }, + { line_code: 'line5', type: 'new' }, + ]; + + const diffFile = { + file_hash: 'abc123', + [INLINE_DIFF_LINES_KEY]: diffLines, + }; + + const discussion = { + diff_file: { file_hash: 'abc123' }, + position: { + line_range: { + start: { line_code: 'line2' }, + end: { line_code: 'line4' }, + }, + }, + }; + + store.diffFiles = [diffFile]; + + const result = store.getLinesForDiscussion({ discussion }); + expect(result).toEqual([ + { line_code: 'line2', type: 'new' }, + { line_code: 'line3', type: 'new' }, + { line_code: 'line4', type: 'new' }, + ]); + }); + + it('excludes old lines from the result', () => { + const diffLines = [ + { line_code: 'line1', type: 'new' }, + { line_code: 'line2', type: OLD_LINE_TYPE }, + { line_code: 'line3', type: 'new' }, + { line_code: 'line4', type: OLD_LINE_TYPE }, + { line_code: 'line5', type: 'new' }, + ]; + + const diffFile = { + file_hash: 'abc123', + [INLINE_DIFF_LINES_KEY]: diffLines, + }; + + const discussion = { + diff_file: { file_hash: 'abc123' }, + position: { + line_range: { + start: { line_code: 'line1' }, + end: { line_code: 'line5' }, + }, + }, + }; + + store.diffFiles = [diffFile]; + + const result = store.getLinesForDiscussion({ discussion }); + expect(result).toEqual([ + { line_code: 'line1', type: 'new' }, + { line_code: 'line3', type: 'new' }, + { line_code: 'line5', type: 'new' }, + ]); + }); + + it('handles case when start and end are the same line', () => { + const diffLines = [ + { line_code: 'line1', type: 'new' }, + { line_code: 'line2', type: 'new' }, + { line_code: 'line3', type: 'new' }, + ]; + + const diffFile = { + file_hash: 'abc123', + [INLINE_DIFF_LINES_KEY]: diffLines, + }; + + const discussion = { + diff_file: { file_hash: 'abc123' }, + position: { + line_range: { + start: { line_code: 'line2' }, + end: { line_code: 'line2' }, + }, + }, + }; + + store.diffFiles = [diffFile]; + + const result = store.getLinesForDiscussion({ discussion }); + expect(result).toEqual([{ line_code: 'line2', type: 'new' }]); + }); + }); + describe('toggleTreeOpen', () => { it('commits TOGGLE_FOLDER_OPEN', () => { return testAction( diff --git a/spec/frontend/notes/components/note_body_spec.js b/spec/frontend/notes/components/note_body_spec.js index 9be25fc4a87e82..b6f90b3a5e0bd4 100644 --- a/spec/frontend/notes/components/note_body_spec.js +++ b/spec/frontend/notes/components/note_body_spec.js @@ -87,6 +87,23 @@ describe('issue_note_body component', () => { expect(wrapper.findComponent(NoteForm).props('saveButtonTitle')).toBe(buttonText); }); + it.each` + type | value + ${'simple'} | ${[]} + ${'complex'} | ${[{ line_code: 'a' }, { line_code: 'b' }]} + `('provides the correct lines to the note form ($type)', ({ value }) => { + createComponent({ + isEditing: true, + autosaveKey, + restoreFromAutosave: true, + lines: value, + }); + + const form = wrapper.findComponent(NoteForm); + + expect(form.props('lines')).toEqual(value); + }); + describe('isInternalNote', () => { beforeEach(() => { wrapper.setProps({ isInternalNote: true }); diff --git a/spec/frontend/notes/components/noteable_discussion_spec.js b/spec/frontend/notes/components/noteable_discussion_spec.js index be8e86c86215e2..ab9c649a0897f9 100644 --- a/spec/frontend/notes/components/noteable_discussion_spec.js +++ b/spec/frontend/notes/components/noteable_discussion_spec.js @@ -36,10 +36,23 @@ Vue.use(PiniaVuePlugin); jest.mock('~/behaviors/markdown/render_gfm'); jest.mock('~/alert'); +function createPinia({ stubActions = true } = {}) { + const pinia = createTestingPinia({ stubActions, plugins: [globalAccessorPlugin] }); + const diffsStore = useLegacyDiffs(); + useNotes().noteableData = noteableDataMock; + useNotes().notesData = notesDataMock; + useNotes().saveNote.mockResolvedValue(); + useNotes().fetchDiscussionDiffLines.mockResolvedValue(); + useBatchComments(); + + return { pinia, diffsStore }; +} + describe('noteable_discussion component', () => { let pinia; let wrapper; let axiosMock; + let diffsStore; const createComponent = ({ discussion = discussionMock } = {}) => { wrapper = mountExtended(NoteableDiscussion, { @@ -49,13 +62,7 @@ describe('noteable_discussion component', () => { }; beforeEach(() => { - pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); - useLegacyDiffs(); - useNotes().noteableData = noteableDataMock; - useNotes().notesData = notesDataMock; - useNotes().saveNote.mockResolvedValue(); - useNotes().fetchDiscussionDiffLines.mockResolvedValue(); - useBatchComments(); + ({ pinia, diffsStore } = createPinia()); axiosMock = new MockAdapter(axios); createComponent(); }); @@ -306,6 +313,61 @@ describe('noteable_discussion component', () => { }); }); + it('includes the original line range when replying', async () => { + wrapper.vm.showReplyForm(); + + await nextTick(); + + const form = wrapper.findComponent(NoteForm); + + expect(form.props('lines')).toEqual([]); + }); + + describe('multi-line comments', () => { + let discussion; + let startCode; + let endCode; + + beforeEach(() => { + ({ pinia, diffsStore } = createPinia({ stubActions: false })); + + const file = getDiffFileMock(); + diffsStore.diffFiles = [file]; + + startCode = file.highlighted_diff_lines[6].line_code; + endCode = file.highlighted_diff_lines[7].line_code; + + discussion = { + ...discussionMock, + diff_file: file, + position: { + line_range: { + start: { + line_code: startCode, + }, + end: { + line_code: endCode, + }, + }, + }, + }; + + createComponent({ discussion }); + }); + + it('includes the original line range when replying to a multiline comment', async () => { + wrapper.vm.showReplyForm(); + await nextTick(); + + const form = wrapper.findComponent(NoteForm); + + expect(form.props('lines')).toEqual([ + expect.objectContaining({ line_code: startCode }), + expect.objectContaining({ line_code: endCode }), + ]); + }); + }); + it('supports direct call on showReplyForm', async () => { const mock = jest.fn(); wrapper = mount(NoteableDiscussion, { diff --git a/spec/frontend/notes/components/noteable_note_spec.js b/spec/frontend/notes/components/noteable_note_spec.js index 65c234bcddc442..9ec74adfe37cb6 100644 --- a/spec/frontend/notes/components/noteable_note_spec.js +++ b/spec/frontend/notes/components/noteable_note_spec.js @@ -5,6 +5,7 @@ import { createTestingPinia } from '@pinia/testing'; import { PiniaVuePlugin } from 'pinia'; import { mountExtended } from 'helpers/vue_test_utils_helper'; import waitForPromises from 'helpers/wait_for_promises'; +import { getDiffFileMock } from 'jest/diffs/mock_data/diff_file'; import NoteActions from '~/notes/components/note_actions.vue'; import NoteBody from '~/notes/components/note_body.vue'; import NoteHeader from '~/notes/components/note_header.vue'; @@ -20,7 +21,7 @@ import { useMockInternalEventsTracking } from 'helpers/tracking_internal_events_ import { globalAccessorPlugin } from '~/pinia/plugins'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useNotes } from '~/notes/store/legacy_notes'; -import { noteableDataMock, notesDataMock, note } from '../mock_data'; +import { noteableDataMock, notesDataMock, discussionMock, note } from '../mock_data'; Vue.use(PiniaVuePlugin); @@ -48,6 +49,7 @@ const singleLineNotePosition = { describe('issue_note', () => { let pinia; + let diffsStore; let wrapper; const REPORT_ABUSE_PATH = '/abuse_reports/add_category'; @@ -79,12 +81,16 @@ describe('issue_note', () => { }); }; - beforeEach(() => { - pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); - useLegacyDiffs(); + const createPinia = ({ stubActions = true } = {}) => { + pinia = createTestingPinia({ stubActions, plugins: [globalAccessorPlugin] }); + diffsStore = useLegacyDiffs(); useNotes().noteableData = noteableDataMock; useNotes().notesData = notesDataMock; useNotes().updateNote.mockResolvedValue(); + }; + + beforeEach(() => { + createPinia(); }); describe('mutiline comments', () => { @@ -194,6 +200,48 @@ describe('issue_note', () => { it('should not render if `line_range` is unavailable', () => { expect(findMultilineComment().exists()).toBe(false); }); + + describe('multi-line line range', () => { + let discussion; + let startCode; + let endCode; + + beforeEach(() => { + createPinia({ stubActions: false }); + + const file = getDiffFileMock(); + diffsStore.diffFiles = [file]; + + startCode = file.highlighted_diff_lines[6].line_code; + endCode = file.highlighted_diff_lines[7].line_code; + + discussion = { + ...discussionMock, + diff_file: file, + position: { + line_range: { + start: { + line_code: startCode, + }, + end: { + line_code: endCode, + }, + }, + }, + }; + + createWrapper({ discussion }); + }); + + it('passes the correct lines to the note body', () => { + const body = findNoteBody(); + + expect(body.props('lines')).toEqual([ + expect.objectContaining({ line_code: startCode }), + expect.objectContaining({ line_code: endCode }), + ]); + }); + }); }); describe('rendering', () => { -- GitLab