From 080f5132436a1d4900f34887cac68a1ecb4026ae Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Sun, 7 Dec 2025 21:52:49 +0400 Subject: [PATCH 1/2] Use code review id instead of client-side diff ID in merge requests Allows for code review status to be preserved in file-by-file mode without loading every single diff Fixes: https://gitlab.com/gitlab-org/gitlab/-/work_items/582822 --- .../javascripts/diffs/components/app.vue | 4 +- .../diffs/components/diff_file_row.vue | 2 +- .../diffs/stores/legacy_diffs/actions.js | 8 ++-- .../diffs/stores/legacy_diffs/mutations.js | 4 +- .../javascripts/diffs/utils/file_reviews.js | 21 ++++----- .../diffs/utils/tree_worker_utils.js | 1 + .../diffs/components/diff_file_row_spec.js | 13 +----- .../diffs/stores/legacy_diffs/actions_spec.js | 22 +++++----- .../stores/legacy_diffs/mutations_spec.js | 10 ++--- .../frontend/diffs/utils/file_reviews_spec.js | 44 ++++++++++--------- 10 files changed, 58 insertions(+), 71 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 1465db9cf3b1ee..84955da3b0fbd4 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -886,7 +886,7 @@ export default { :file="item" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[item.id]" + :reviewed="fileReviews[item.code_review_id || item.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" @@ -908,7 +908,7 @@ export default { :file="file" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[file.id]" + :reviewed="fileReviews[file.code_review_id || file.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/diffs/components/diff_file_row.vue b/app/assets/javascripts/diffs/components/diff_file_row.vue index 061588373b4330..94234af58842af 100644 --- a/app/assets/javascripts/diffs/components/diff_file_row.vue +++ b/app/assets/javascripts/diffs/components/diff_file_row.vue @@ -39,7 +39,7 @@ export default { return !this.hideFileStats && this.file.type === 'blob'; }, fileClasses() { - return this.file.type === 'blob' && !this.viewedFiles[this.file.id || this.file.fileHash] + return this.file.type === 'blob' && !this.viewedFiles[this.file.codeReviewId || this.file.id] ? 'gl-font-bold' : 'gl-text-subtle'; }, diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index 00327a853df9a9..f35d9a1ebb1867 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -116,10 +116,8 @@ export function setBaseConfig(options) { perPage, }); - Array.from(new Set(Object.values(mrReviews).flat())).forEach((id) => { - const viewedId = id.replace(/^hash:/, ''); - - this[types.SET_DIFF_FILE_VIEWED]({ id: viewedId, seen: true }); + Array.from(new Set(Object.values(mrReviews).flat())).forEach((codeReviewId) => { + this[types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen: true }); }); } @@ -1091,7 +1089,7 @@ export function reviewFile({ file, reviewed = true }) { setReviewsForMergeRequest(mrPath, reviews); - this[types.SET_DIFF_FILE_VIEWED]({ id: file.id, seen: reviewed }); + this[types.SET_DIFF_FILE_VIEWED]({ codeReviewId: file.code_review_id, seen: reviewed }); this[types.SET_MR_FILE_REVIEWS](reviews); } diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js index ac417d6d3bfe90..f0a5b78aa47e13 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js @@ -296,10 +296,10 @@ export default { [types.SET_CURRENT_DIFF_FILE](fileId) { this.currentDiffFileId = fileId; }, - [types.SET_DIFF_FILE_VIEWED]({ id, seen }) { + [types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen }) { this.viewedDiffFileIds = { ...this.viewedDiffFileIds, - [id]: seen, + [codeReviewId]: seen, }; }, [types.OPEN_DIFF_FILE_COMMENT_FORM](formData) { diff --git a/app/assets/javascripts/diffs/utils/file_reviews.js b/app/assets/javascripts/diffs/utils/file_reviews.js index 581d0b6055ba2c..9aab9f9eef646a 100644 --- a/app/assets/javascripts/diffs/utils/file_reviews.js +++ b/app/assets/javascripts/diffs/utils/file_reviews.js @@ -2,17 +2,14 @@ function getFileReviewsKey(mrPath) { return `${mrPath}-file-reviews`; } -export function isFileReviewed(reviews, file) { - const fileReviews = reviews[file.file_identifier_hash]; - - return file?.id && fileReviews?.length ? new Set(fileReviews).has(file.id) : false; -} - export function reviewStatuses(files, reviews) { - return files.reduce((flat, file) => { + return files.reduce((acc, file) => { + const fileReviews = reviews[file.file_identifier_hash]; + if (!fileReviews) return acc; return { - ...flat, - [file.id]: isFileReviewed(reviews, file), + ...acc, + [file.id]: fileReviews.includes(file.id), + [file.code_review_id]: fileReviews.includes(file.code_review_id), }; }, {}); } @@ -39,7 +36,7 @@ export function setReviewsForMergeRequest(mrPath, reviews) { } export function reviewable(file) { - return Boolean(file.id) && Boolean(file.file_identifier_hash); + return Boolean(file.code_review_id) && Boolean(file.file_identifier_hash); } export function markFileReview(reviews, file, reviewed = true) { @@ -51,11 +48,11 @@ export function markFileReview(reviews, file, reviewed = true) { fileReviews = new Set(usableReviews[file.file_identifier_hash] || []); if (reviewed) { - fileReviews.add(file.id); - fileReviews.add(`hash:${file.file_hash}`); + fileReviews.add(file.code_review_id); } else { fileReviews.delete(file.id); fileReviews.delete(`hash:${file.file_hash}`); + fileReviews.delete(file.code_review_id); } updatedReviews[file.file_identifier_hash] = Array.from(fileReviews); diff --git a/app/assets/javascripts/diffs/utils/tree_worker_utils.js b/app/assets/javascripts/diffs/utils/tree_worker_utils.js index e1e3495a51f6c7..e8b5a28ca11bf6 100644 --- a/app/assets/javascripts/diffs/utils/tree_worker_utils.js +++ b/app/assets/javascripts/diffs/utils/tree_worker_utils.js @@ -93,6 +93,7 @@ export const generateTreeList = (files) => { tempFile: file.new_file, deleted: file.deleted_file, fileHash: file.file_hash, + codeReviewId: file.code_review_id, addedLines: file.added_lines, removedLines: file.removed_lines, parentPath: parent ? `${parent.path}/` : '/', diff --git a/spec/frontend/diffs/components/diff_file_row_spec.js b/spec/frontend/diffs/components/diff_file_row_spec.js index 028bfb9b339759..bf1ff4c73e1124 100644 --- a/spec/frontend/diffs/components/diff_file_row_spec.js +++ b/spec/frontend/diffs/components/diff_file_row_spec.js @@ -65,7 +65,7 @@ describe('Diff File Row component', () => { createComponent({ file: { type: fileType, - id: '#123456789', + codeReviewId: '#123456789', }, level: 0, hideFileStats: false, @@ -103,15 +103,4 @@ describe('Diff File Row component', () => { expect(wrapper.classes('is-active')).toBe(true); }); - - it('marks viewed files', () => { - createComponent({ - level: 0, - file: { fileHash: '123', type: 'blob' }, - viewedFiles: { 123: true }, - hideFileStats: false, - }); - - expect(wrapper.findComponent(FileRow).props('fileClasses')).toContain('gl-text-subtle'); - }); }); diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index e70f39fc77bbd3..3f01d6877c339e 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -114,8 +114,8 @@ describe('legacyDiffs actions', () => { const dismissEndpoint = '/-/user_callouts'; const showSuggestPopover = false; const mrReviews = { - a: ['z', 'hash:a'], - b: ['y', 'hash:a'], + a: ['z', 'a'], + b: ['y', 'a'], }; const diffViewType = 'inline'; @@ -161,15 +161,15 @@ describe('legacyDiffs actions', () => { }, { type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'z', seen: true }, + payload: { codeReviewId: 'z', seen: true }, }, { type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'a', seen: true }, + payload: { codeReviewId: 'a', seen: true }, }, { type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'y', seen: true }, + payload: { codeReviewId: 'y', seen: true }, }, ], [], @@ -2207,20 +2207,18 @@ describe('legacyDiffs actions', () => { describe('reviewFile', () => { const file = { - id: '123', + code_review_id: '123', file_hash: 'xyz', file_identifier_hash: 'abc', load_collapsed_diff_url: 'gitlab-org/gitlab-test/-/merge_requests/1/diffs', }; it.each` - reviews | diffFile | reviewed - ${{ abc: ['123', 'hash:xyz'] }} | ${file} | ${true} - ${{}} | ${file} | ${false} + reviews | diffFile | reviewed + ${{ abc: ['123'] }} | ${file} | ${true} + ${{}} | ${file} | ${false} `( 'sets reviews ($reviews) to localStorage and state for file $file if it is marked reviewed=$reviewed', ({ reviews, diffFile, reviewed }) => { - store.$patch({ mrReviews: { abc: ['123'] } }); - store.reviewFile({ file: diffFile, reviewed, @@ -2228,7 +2226,7 @@ describe('legacyDiffs actions', () => { expect(localStorage.setItem).toHaveBeenCalledTimes(1); expect(localStorage.setItem).toHaveBeenCalledWith( - 'gitlab-org/gitlab-test/-/merge_requests/1-file-reviews', + 'code-review-gitlab-org/gitlab-test/-/merge_requests/1', JSON.stringify(reviews), ); expect(store[types.SET_MR_FILE_REVIEWS]).toHaveBeenCalledWith(reviews); diff --git a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js index a74b674bcc5297..0b61c9ae23b52b 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js @@ -996,11 +996,11 @@ describe('DiffsStoreMutations', () => { }); it.each` - id | bool | outcome - ${'abc'} | ${true} | ${{ 123: true, abc: true }} - ${'123'} | ${false} | ${{ 123: false }} - `('sets the viewed files list to $bool for the id $id', ({ id, bool, outcome }) => { - store[types.SET_DIFF_FILE_VIEWED]({ id, seen: bool }); + codeReviewId | bool | outcome + ${'abc'} | ${true} | ${{ 123: true, abc: true }} + ${'123'} | ${false} | ${{ 123: false }} + `('sets the viewed files list to $bool for the id $id', ({ codeReviewId, bool, outcome }) => { + store[types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen: bool }); expect(store.viewedDiffFileIds).toEqual(outcome); }); diff --git a/spec/frontend/diffs/utils/file_reviews_spec.js b/spec/frontend/diffs/utils/file_reviews_spec.js index ccd27a5ae3e3e0..a54b802ff20a56 100644 --- a/spec/frontend/diffs/utils/file_reviews_spec.js +++ b/spec/frontend/diffs/utils/file_reviews_spec.js @@ -11,14 +11,14 @@ import { function getDefaultReviews() { return { - abc: ['123', 'hash:xyz', '098', 'hash:uvw'], + abc: ['123', '098'], }; } describe('File Review(s) utilities', () => { const mrPath = 'my/fake/mr/42'; - const storageKey = `${mrPath}-file-reviews`; - const file = { id: '123', file_hash: 'xyz', file_identifier_hash: 'abc' }; + const storageKey = `code-review-${mrPath}`; + const file = { code_review_id: '123', file_hash: 'xyz', file_identifier_hash: 'abc' }; const storedValue = JSON.stringify(getDefaultReviews()); let reviews; @@ -31,9 +31,9 @@ describe('File Review(s) utilities', () => { describe('isFileReviewed', () => { it.each` - description | diffFile | fileReviews - ${'the file does not have an `id`'} | ${{ ...file, id: undefined }} | ${getDefaultReviews()} - ${'there are no reviews for the file'} | ${file} | ${{ ...getDefaultReviews(), abc: undefined }} + description | diffFile | fileReviews + ${'the file does not have an `id`'} | ${{ ...file, code_review_id: undefined }} | ${getDefaultReviews()} + ${'there are no reviews for the file'} | ${file} | ${{ ...getDefaultReviews(), abc: undefined }} `('returns `false` if $description', ({ diffFile, fileReviews }) => { expect(isFileReviewed(fileReviews, diffFile)).toBe(false); }); @@ -44,8 +44,8 @@ describe('File Review(s) utilities', () => { }); describe('reviewStatuses', () => { - const file1 = { id: '123', hash: 'xyz', file_identifier_hash: 'abc' }; - const file2 = { id: '098', hash: 'uvw', file_identifier_hash: 'abc' }; + const file1 = { code_review_id: '123', hash: 'xyz', file_identifier_hash: 'abc' }; + const file2 = { code_review_id: '098', hash: 'uvw', file_identifier_hash: 'abc' }; it.each` mrReviews | files | fileReviews @@ -112,11 +112,11 @@ describe('File Review(s) utilities', () => { response | diffFile | description ${true} | ${file} | ${'has an `.id` and a `.file_identifier_hash`'} ${false} | ${{ file_identifier_hash: 'abc' }} | ${'does not have an `.id`'} - ${false} | ${{ ...file, id: undefined }} | ${'has an undefined `.id`'} - ${false} | ${{ ...file, id: null }} | ${'has a null `.id`'} - ${false} | ${{ ...file, id: 0 }} | ${'has an `.id` set to 0'} - ${false} | ${{ ...file, id: false }} | ${'has an `.id` set to false'} - ${false} | ${{ id: '123' }} | ${'does not have a `.file_identifier_hash`'} + ${false} | ${{ ...file, code_review_id: undefined }} | ${'has an undefined `.id`'} + ${false} | ${{ ...file, code_review_id: null }} | ${'has a null `.id`'} + ${false} | ${{ ...file, code_review_id: 0 }} | ${'has an `.id` set to 0'} + ${false} | ${{ ...file, code_review_id: false }} | ${'has an `.id` set to false'} + ${false} | ${{ code_review_id: '123' }} | ${'does not have a `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: undefined }} | ${'has an undefined `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: null }} | ${'has a null `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: 0 }} | ${'has a `.file_identifier_hash` set to 0'} @@ -128,7 +128,7 @@ describe('File Review(s) utilities', () => { describe('markFileReview', () => { it("adds a review when there's nothing that already exists", () => { - expect(markFileReview(null, file)).toStrictEqual({ abc: ['123', 'hash:xyz'] }); + expect(markFileReview(null, file)).toStrictEqual({ abc: ['123'] }); }); it("overwrites an existing review if it's for the same file (identifier hash)", () => { @@ -136,21 +136,21 @@ describe('File Review(s) utilities', () => { }); it('removes a review from the list when `reviewed` is `false`', () => { - expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098', 'hash:uvw'] }); + expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098'] }); }); it('adds a new review if the file ID is new', () => { - const updatedFile = { ...file, id: '098', file_hash: 'uvw' }; - const allReviews = markFileReview({ abc: ['123', 'hash:xyz'] }, updatedFile); + const updatedFile = { ...file, code_review_id: '098', file_hash: 'uvw' }; + const allReviews = markFileReview({ abc: ['123'] }, updatedFile); expect(allReviews).toStrictEqual(getDefaultReviews()); - expect(allReviews.abc).toStrictEqual(['123', 'hash:xyz', '098', 'hash:uvw']); + expect(allReviews.abc).toStrictEqual(['123', '098']); }); it.each` description | diffFile ${'missing an `.id`'} | ${{ file_identifier_hash: 'abc' }} - ${'missing a `.file_identifier_hash`'} | ${{ id: '123' }} + ${'missing a `.file_identifier_hash`'} | ${{ code_review_id: '123' }} `("doesn't modify the reviews if the file is $description", ({ diffFile }) => { expect(markFileReview(reviews, diffFile)).toStrictEqual(getDefaultReviews()); }); @@ -158,7 +158,11 @@ describe('File Review(s) utilities', () => { it('removes the file key if there are no more reviews for it', () => { let updated = markFileReview(reviews, file, false); - updated = markFileReview(updated, { ...file, id: '098', file_hash: 'uvw' }, false); + updated = markFileReview( + updated, + { ...file, code_review_id: '098', file_hash: 'uvw' }, + false, + ); expect(updated).toStrictEqual({}); }); -- GitLab From 04ec49c9679625cbdf7ebbc4093f446ccb1ca500 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Sat, 13 Dec 2025 03:11:14 +0400 Subject: [PATCH 2/2] Refactor to use CodeReview store as SSOT for viewed checkbox --- .../javascripts/diffs/components/app.vue | 21 ++-- .../diffs/components/diff_file_header.vue | 12 +- .../diffs/components/diff_file_row.vue | 3 +- .../diffs/components/tree_list.vue | 5 +- .../javascripts/diffs/stores/code_review.js | 64 +++++++++++ .../diffs/stores/legacy_diffs/actions.js | 17 --- .../diffs/stores/legacy_diffs/index.js | 2 - .../diffs/stores/legacy_diffs/mutations.js | 11 -- .../javascripts/diffs/utils/file_reviews.js | 21 ++-- app/assets/javascripts/mr_notes/init.js | 6 +- ee/spec/frontend/diffs/components/app_spec.js | 2 + spec/frontend/diffs/components/app_spec.js | 25 ++-- .../diffs/components/diff_file_header_spec.js | 24 ++-- .../diffs/components/tree_list_spec.js | 10 +- .../frontend/diffs/stores/code_review_spec.js | 107 ++++++++++++++++++ .../diffs/stores/legacy_diffs/actions_spec.js | 47 -------- .../stores/legacy_diffs/mutations_spec.js | 31 ----- .../frontend/diffs/utils/file_reviews_spec.js | 44 ++++--- 18 files changed, 258 insertions(+), 194 deletions(-) create mode 100644 app/assets/javascripts/diffs/stores/code_review.js create mode 100644 spec/frontend/diffs/stores/code_review_spec.js diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 84955da3b0fbd4..02eac66f609071 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -31,6 +31,7 @@ import { useNotes } from '~/notes/store/legacy_notes'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; import { useBatchComments } from '~/batch_comments/store'; import { querySelectionClosest } from '~/lib/utils/selection'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { sortFindingsByFile } from '../utils/sort_findings_by_file'; import { ALERT_OVERFLOW_HIDDEN, @@ -51,7 +52,6 @@ import { } from '../constants'; import { isCollapsed } from '../utils/diff_file'; import diffsEventHub from '../event_hub'; -import { reviewStatuses } from '../utils/file_reviews'; import { diffsApp } from '../utils/performance'; import { updateChangesTabCount, extractFileHash } from '../utils/merge_request'; import { queueRedisHllEvents } from '../utils/queue_events'; @@ -238,7 +238,6 @@ export default { 'isTreeLoaded', 'hasConflicts', 'viewDiffsFileByFile', - 'mrReviews', 'renderTreeList', 'showWhitespace', 'addedLines', @@ -254,6 +253,7 @@ export default { ...mapState(useNotes, ['discussions', 'isNotesFetched', 'getNoteableData']), ...mapState(useFileBrowser, ['fileBrowserVisible']), ...mapState(useFindingsDrawer, ['activeDrawer']), + ...mapState(useCodeReview, ['reviewedIds']), diffs() { if (!this.viewDiffsFileByFile) { return this.diffFiles; @@ -307,9 +307,6 @@ export default { return visible; }, - fileReviews() { - return reviewStatuses(this.diffFiles, this.mrReviews); - }, renderFileTree() { return this.renderDiffFiles && this.fileBrowserVisible; }, @@ -469,7 +466,6 @@ export default { 'setDiffViewType', 'setShowWhitespace', 'goToFile', - 'reviewFile', 'setFileCollapsedByUser', 'toggleTreeOpen', ]), @@ -488,9 +484,12 @@ export default { const activeFile = this.diffFiles.find((file) => file.file_hash === this.currentDiffFileId); if (activeFile) { - const reviewed = !this.fileReviews[activeFile.id]; - this.reviewFile({ file: activeFile, reviewed }); - + const reviewed = !( + useCodeReview().reviewedIds[activeFile.code_review_id] || + useCodeReview().reviewedIds[activeFile.id] + ); + useCodeReview().removeId(activeFile.id); + useCodeReview().setReviewed(activeFile.code_review_id, reviewed); this.setFileCollapsedByUser({ filePath: activeFile.file_path, collapsed: reviewed }); } }, @@ -886,7 +885,7 @@ export default { :file="item" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[item.code_review_id || item.id]" + :reviewed="reviewedIds[item.code_review_id] || reviewedIds[item.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" @@ -908,7 +907,7 @@ export default { :file="file" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[file.code_review_id || file.id]" + :reviewed="reviewedIds[file.code_review_id] || reviewedIds[file.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index e87a4bb5074f50..0293c2c9b351a3 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -21,15 +21,14 @@ import { truncateSha } from '~/lib/utils/text_utility'; import { sanitize } from '~/lib/dompurify'; import { __, s__, sprintf } from '~/locale'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; - import { createFileUrl, fileContentsId } from '~/diffs/components/diff_row_utils'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useNotes } from '~/notes/store/legacy_notes'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { DIFF_FILE_AUTOMATIC_COLLAPSE } from '../constants'; import diffsEventHub from '../event_hub'; import { DIFF_FILE_HEADER } from '../i18n'; import { collapsedType, isCollapsed } from '../utils/diff_file'; -import { reviewable } from '../utils/file_reviews'; import DiffStats from './diff_stats.vue'; const createHotkeyHtml = (key) => ``; @@ -191,7 +190,7 @@ export default { ); }, isReviewable() { - return reviewable(this.diffFile); + return Boolean(this.diffFile.code_review_id) && Boolean(this.diffFile.file_identifier_hash); }, externalUrlLabel() { return sprintf(__('View on %{url}'), { url: this.diffFile.formatted_external_url }); @@ -263,7 +262,6 @@ export default { 'toggleFileDiscussionWrappers', 'toggleFullDiff', 'setCurrentFileHash', - 'reviewFile', 'setFileCollapsedByUser', 'setFileForcedOpen', 'toggleFileCommentForm', @@ -311,7 +309,11 @@ export default { const closed = !open; const reviewed = newReviewedStatus; - this.reviewFile({ file: this.diffFile, reviewed }); + // diffFile.id is generated on the frontend, code_review_id is generated on the backend + // we need to leave only code_review_id as our single source of truth + // this is done to prevent legacy id from persisting in the local storage + useCodeReview().removeId(this.diffFile.id); + useCodeReview().setReviewed(this.diffFile.code_review_id, reviewed); if (reviewed && autoCollapsed) { this.setFileCollapsedByUser({ diff --git a/app/assets/javascripts/diffs/components/diff_file_row.vue b/app/assets/javascripts/diffs/components/diff_file_row.vue index 94234af58842af..606f86a1fb67c8 100644 --- a/app/assets/javascripts/diffs/components/diff_file_row.vue +++ b/app/assets/javascripts/diffs/components/diff_file_row.vue @@ -39,7 +39,8 @@ export default { return !this.hideFileStats && this.file.type === 'blob'; }, fileClasses() { - return this.file.type === 'blob' && !this.viewedFiles[this.file.codeReviewId || this.file.id] + return this.file.type === 'blob' && + !(this.viewedFiles[this.file.codeReviewId] || this.viewedFiles[this.file.id]) ? 'gl-font-bold' : 'gl-text-subtle'; }, diff --git a/app/assets/javascripts/diffs/components/tree_list.vue b/app/assets/javascripts/diffs/components/tree_list.vue index aaa7697632560f..a9472ce292d3f4 100644 --- a/app/assets/javascripts/diffs/components/tree_list.vue +++ b/app/assets/javascripts/diffs/components/tree_list.vue @@ -13,6 +13,7 @@ import { RecycleScroller } from 'vendor/vue-virtual-scroller'; import { isElementClipped } from '~/lib/utils/common_utils'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { MR_FOCUS_FILE_BROWSER } from '~/behaviors/shortcuts/keybindings'; +import { useCodeReview } from '~/diffs/stores/code_review'; import DiffFileRow from './diff_file_row.vue'; export default { @@ -61,12 +62,12 @@ export default { ...mapState(useLegacyDiffs, [ 'renderTreeList', 'currentDiffFileId', - 'viewedDiffFileIds', 'fileTree', 'allBlobs', 'linkedFile', 'flatBlobsList', ]), + ...mapState(useCodeReview, ['reviewedIds']), flatUngroupedList() { return this.flatBlobsList.reduce((acc, blob, index) => { const loading = this.isLoading(blob.fileHash); @@ -296,7 +297,7 @@ export default { { + this.reviewedIds[id] = true; + }); + this.reviewedIds = { ...this.reviewedIds }; + }, + restoreFromLegacyMrReviews() { + const reviewsForMr = localStorage.getItem(`${this.mrPath}-file-reviews`); + if (!reviewsForMr) return; + const reviews = JSON.parse(reviewsForMr); + Object.values(reviews).forEach((values) => { + values.forEach((value) => { + // eslint-disable-next-line @gitlab/require-i18n-strings + if (!value.startsWith('hash:')) this.reviewedIds[value] = true; + }); + }); + this.reviewedIds = { ...this.reviewedIds }; + localStorage.removeItem(`${this.mrPath}-file-reviews`); + this.autosave(); + }, + setReviewed(id, reviewed) { + this.reviewedIds = { ...this.reviewedIds, [id]: reviewed }; + this.autosave(); + }, + removeId(id) { + delete this.reviewedIds[id]; + }, + autosave() { + const ids = this.markedAsViewedIds; + if (ids.length) { + localStorage.setItem(this.autosaveKey, JSON.stringify(ids)); + } else { + localStorage.removeItem(this.autosaveKey); + } + }, + }, + getters: { + markedAsViewedIds() { + return Object.keys(this.reviewedIds).reduce((acc, key) => { + if (this.reviewedIds[key]) acc.push(key); + return acc; + }, []); + }, + autosaveKey() { + return `code-review-${this.mrPath}`; + }, + }, +}); diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index f35d9a1ebb1867..0f233795ff1db1 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -67,7 +67,6 @@ import { ENCODED_FILE_PATHS_MESSAGE, } from '../../i18n'; import eventHub from '../../event_hub'; -import { markFileReview, setReviewsForMergeRequest } from '../../utils/file_reviews'; import { getDerivedMergeRequestInformation } from '../../utils/merge_request'; import { queueRedisHllEvents } from '../../utils/queue_events'; import * as types from '../../store/mutation_types'; @@ -95,7 +94,6 @@ export function setBaseConfig(options) { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, } = options; @@ -111,14 +109,9 @@ export function setBaseConfig(options) { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, }); - - Array.from(new Set(Object.values(mrReviews).flat())).forEach((codeReviewId) => { - this[types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen: true }); - }); } export async function prefetchSingleFile(treeEntry) { @@ -1083,16 +1076,6 @@ export function setFileByFile({ fileByFile }) { }); } -export function reviewFile({ file, reviewed = true }) { - const { mrPath } = getDerivedMergeRequestInformation({ endpoint: file.load_collapsed_diff_url }); - const reviews = markFileReview(this.mrReviews, file, reviewed); - - setReviewsForMergeRequest(mrPath, reviews); - - this[types.SET_DIFF_FILE_VIEWED]({ codeReviewId: file.code_review_id, seen: reviewed }); - this[types.SET_MR_FILE_REVIEWS](reviews); -} - export function disableVirtualScroller() { this[types.DISABLE_VIRTUAL_SCROLLING](); } diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/index.js b/app/assets/javascripts/diffs/stores/legacy_diffs/index.js index 27b8f829cbb894..714f890f025a50 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/index.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/index.js @@ -33,7 +33,6 @@ export const useLegacyDiffs = defineStore('legacyDiffs', { treeEntries: {}, currentDiffFileId: '', projectPath: '', - viewedDiffFileIds: {}, commentForms: [], highlightedRow: null, renderTreeList: true, @@ -43,7 +42,6 @@ export const useLegacyDiffs = defineStore('legacyDiffs', { dismissEndpoint: '', showSuggestPopover: true, defaultSuggestionCommitMessage: '', - mrReviews: {}, latestDiff: true, virtualScrollerDisabled: false, linkedFileHash: null, diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js index f0a5b78aa47e13..523027c9cc2e12 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js @@ -32,7 +32,6 @@ export default { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, } = options; @@ -48,7 +47,6 @@ export default { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, }); @@ -296,12 +294,6 @@ export default { [types.SET_CURRENT_DIFF_FILE](fileId) { this.currentDiffFileId = fileId; }, - [types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen }) { - this.viewedDiffFileIds = { - ...this.viewedDiffFileIds, - [codeReviewId]: seen, - }; - }, [types.OPEN_DIFF_FILE_COMMENT_FORM](formData) { this.commentForms.push({ ...formData, @@ -400,9 +392,6 @@ export default { [types.SET_FILE_BY_FILE](fileByFile) { this.viewDiffsFileByFile = fileByFile; }, - [types.SET_MR_FILE_REVIEWS](newReviews) { - this.mrReviews = newReviews; - }, [types.DISABLE_VIRTUAL_SCROLLING]() { this.virtualScrollerDisabled = true; }, diff --git a/app/assets/javascripts/diffs/utils/file_reviews.js b/app/assets/javascripts/diffs/utils/file_reviews.js index 9aab9f9eef646a..581d0b6055ba2c 100644 --- a/app/assets/javascripts/diffs/utils/file_reviews.js +++ b/app/assets/javascripts/diffs/utils/file_reviews.js @@ -2,14 +2,17 @@ function getFileReviewsKey(mrPath) { return `${mrPath}-file-reviews`; } +export function isFileReviewed(reviews, file) { + const fileReviews = reviews[file.file_identifier_hash]; + + return file?.id && fileReviews?.length ? new Set(fileReviews).has(file.id) : false; +} + export function reviewStatuses(files, reviews) { - return files.reduce((acc, file) => { - const fileReviews = reviews[file.file_identifier_hash]; - if (!fileReviews) return acc; + return files.reduce((flat, file) => { return { - ...acc, - [file.id]: fileReviews.includes(file.id), - [file.code_review_id]: fileReviews.includes(file.code_review_id), + ...flat, + [file.id]: isFileReviewed(reviews, file), }; }, {}); } @@ -36,7 +39,7 @@ export function setReviewsForMergeRequest(mrPath, reviews) { } export function reviewable(file) { - return Boolean(file.code_review_id) && Boolean(file.file_identifier_hash); + return Boolean(file.id) && Boolean(file.file_identifier_hash); } export function markFileReview(reviews, file, reviewed = true) { @@ -48,11 +51,11 @@ export function markFileReview(reviews, file, reviewed = true) { fileReviews = new Set(usableReviews[file.file_identifier_hash] || []); if (reviewed) { - fileReviews.add(file.code_review_id); + fileReviews.add(file.id); + fileReviews.add(`hash:${file.file_hash}`); } else { fileReviews.delete(file.id); fileReviews.delete(`hash:${file.file_hash}`); - fileReviews.delete(file.code_review_id); } updatedReviews[file.file_identifier_hash] = Array.from(fileReviews); diff --git a/app/assets/javascripts/mr_notes/init.js b/app/assets/javascripts/mr_notes/init.js index 431b2f5f9b5162..c526498d301983 100644 --- a/app/assets/javascripts/mr_notes/init.js +++ b/app/assets/javascripts/mr_notes/init.js @@ -4,13 +4,13 @@ import eventHub from '~/notes/event_hub'; import { initDiscussionCounter } from '~/mr_notes/discussion_counter'; import { initOverviewTabCounter } from '~/mr_notes/init_count'; import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request'; -import { getReviewsForMergeRequest } from '~/diffs/utils/file_reviews'; import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; import { useNotes } from '~/notes/store/legacy_notes'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useBatchComments } from '~/batch_comments/store'; import { useMrNotes } from '~/mr_notes/store/legacy_mr_notes'; import { pinia } from '~/pinia/instance'; +import { useCodeReview } from '~/diffs/stores/code_review'; function setupMrNotesState(notesDataset, diffsDataset = {}) { const noteableData = JSON.parse(notesDataset.noteableData); @@ -41,11 +41,13 @@ function setupMrNotesState(notesDataset, diffsDataset = {}) { showSuggestPopover: parseBoolean(diffsDataset.showSuggestPopover), viewDiffsFileByFile: parseBoolean(diffsDataset.fileByFileDefault), defaultSuggestionCommitMessage: diffsDataset.defaultSuggestionCommitMessage, - mrReviews: getReviewsForMergeRequest(mrPath), diffViewType: getParameterValues('view')[0] || getCookie(DIFF_VIEW_COOKIE_NAME) || INLINE_DIFF_VIEW_TYPE, perPage: Number(diffsDataset.perPage), }); + useCodeReview().setMrPath(mrPath); + useCodeReview().restoreFromAutosave(); + useCodeReview().restoreFromLegacyMrReviews(); } export function initMrStateLazyLoad() { diff --git a/ee/spec/frontend/diffs/components/app_spec.js b/ee/spec/frontend/diffs/components/app_spec.js index 21b51812593280..0b47dd5d7958bf 100644 --- a/ee/spec/frontend/diffs/components/app_spec.js +++ b/ee/spec/frontend/diffs/components/app_spec.js @@ -14,6 +14,7 @@ import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useNotes } from '~/notes/store/legacy_notes'; import { useBatchComments } from '~/batch_comments/store'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { codeQualityNewErrorsHandler, SASTParsedHandler, @@ -56,6 +57,7 @@ describe('diffs/components/app', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); store = useLegacyDiffs(); store.isLoading = false; diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 439b8d3d770bac..cac9761347c539 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -51,6 +51,7 @@ import { useNotes } from '~/notes/store/legacy_notes'; import { useBatchComments } from '~/batch_comments/store'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; import { querySelectionClosest } from '~/lib/utils/selection'; +import { useCodeReview } from '~/diffs/stores/code_review'; import diffsMockData from '../mock_data/merge_request_diffs'; const TEST_ENDPOINT = `${TEST_HOST}/diff/endpoint`; @@ -102,6 +103,7 @@ describe('diffs/components/app', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); store = useLegacyDiffs(); store.isLoading = false; store.isTreeLoaded = true; @@ -332,24 +334,18 @@ describe('diffs/components/app', () => { beforeEach(() => { store.diffFiles = [ - { id: 1, file_hash: '111', file_path: '111.js' }, - { id: 2, file_hash: '222', file_path: '222.js' }, + { id: 1, file_hash: '111', file_path: '111.js', code_review_id: '111' }, + { id: 2, file_hash: '222', file_path: '222.js', code_review_id: '222' }, ]; createComponent(); }); it('marks active file as reviewed when triggering review toggle shortcut', () => { store.currentDiffFileId = '111'; - store.fileReviews = { 1: false }; toggleReview(); - expect(store.reviewFile).toHaveBeenCalledWith({ - file: store.diffFiles[0], - reviewed: true, - }); - expect(store.reviewFile).toHaveBeenCalledTimes(1); - + expect(useCodeReview().setReviewed).toHaveBeenCalledWith('111', true); expect(store.setFileCollapsedByUser).toHaveBeenCalledWith({ filePath: '111.js', collapsed: true, @@ -359,16 +355,11 @@ describe('diffs/components/app', () => { it('marks active file as unreviewed when triggering review toggle shortcut again', () => { store.currentDiffFileId = '222'; - jest.spyOn(wrapper.vm, 'fileReviews', 'get').mockReturnValue({ 2: true }); + useCodeReview().reviewedIds = { 222: true }; toggleReview(); - expect(store.reviewFile).toHaveBeenCalledWith({ - file: store.diffFiles[1], - reviewed: false, - }); - expect(store.reviewFile).toHaveBeenCalledTimes(1); - + expect(useCodeReview().setReviewed).toHaveBeenCalledWith('222', false); expect(store.setFileCollapsedByUser).toHaveBeenCalledWith({ filePath: '222.js', collapsed: false, @@ -381,7 +372,7 @@ describe('diffs/components/app', () => { toggleReview(); - expect(store.reviewFile).not.toHaveBeenCalled(); + expect(useCodeReview().setReviewed).not.toHaveBeenCalled(); expect(store.setFileCollapsedByUser).not.toHaveBeenCalled(); }); }); diff --git a/spec/frontend/diffs/components/diff_file_header_spec.js b/spec/frontend/diffs/components/diff_file_header_spec.js index 5fcdcc60b973d5..56c12f988f8fe5 100644 --- a/spec/frontend/diffs/components/diff_file_header_spec.js +++ b/spec/frontend/diffs/components/diff_file_header_spec.js @@ -15,6 +15,7 @@ import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { globalAccessorPlugin } from '~/pinia/plugins'; import { useNotes } from '~/notes/store/legacy_notes'; import diffsEventHub from '~/diffs/event_hub'; +import { useCodeReview } from '~/diffs/stores/code_review'; import diffDiscussionsMockData from '../mock_data/diff_discussions'; jest.mock('~/lib/utils/common_utils', () => ({ @@ -45,6 +46,7 @@ const createDiffFile = () => ({ readable_text: true, icon: 'doc-text', }, + code_review_id: '123', }); Vue.use(PiniaVuePlugin); @@ -88,6 +90,7 @@ describe('DiffFileHeader component', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); useLegacyDiffs().diffFiles = [createDiffFile()]; useNotes(); }); @@ -541,28 +544,27 @@ describe('DiffFileHeader component', () => { describe('file reviews', () => { it('calls the action to set the new review', () => { jest.spyOn(document.activeElement, 'blur'); + const diffFile = { + ...createDiffFile(), + viewer: { + ...createDiffFile().viewer, + automaticallyCollapsed: false, + manuallyCollapsed: null, + }, + }; createComponent({ props: { - diffFile: { - ...createDiffFile(), - viewer: { - ...createDiffFile().viewer, - automaticallyCollapsed: false, - manuallyCollapsed: null, - }, - }, + diffFile, showLocalFileReviews: true, addMergeRequestButtons: true, }, }); - const file = wrapper.vm.diffFile; - findReviewFileCheckbox().vm.$emit('change', true); expect(document.activeElement.blur).toHaveBeenCalled(); - expect(useLegacyDiffs().reviewFile).toHaveBeenCalledWith({ file, reviewed: true }); + expect(useCodeReview().setReviewed).toHaveBeenCalledWith(diffFile.code_review_id, true); }); it.each` diff --git a/spec/frontend/diffs/components/tree_list_spec.js b/spec/frontend/diffs/components/tree_list_spec.js index a4ee48353b2191..c104709a8e41d2 100644 --- a/spec/frontend/diffs/components/tree_list_spec.js +++ b/spec/frontend/diffs/components/tree_list_spec.js @@ -12,6 +12,7 @@ import { isElementClipped } from '~/lib/utils/common_utils'; import { globalAccessorPlugin } from '~/pinia/plugins'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { getDiffFileMock } from 'jest/diffs/mock_data/diff_file'; +import { useCodeReview } from '~/diffs/stores/code_review'; jest.mock('~/lib/utils/common_utils'); @@ -48,6 +49,7 @@ describe('Diffs tree list component', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin], stubActions: false }); + useCodeReview(); useLegacyDiffs().isTreeLoaded = true; useLegacyDiffs().diffFiles = [getDiffFileMock()]; useLegacyDiffs().addedLines = 10; @@ -304,18 +306,18 @@ describe('Diffs tree list component', () => { }); describe('with viewedDiffFileIds', () => { - const viewedDiffFileIds = { fileId: '#12345' }; + const reviewedIds = { 12345: true }; beforeEach(() => { setupFilesInState(); - useLegacyDiffs().viewedDiffFileIds = viewedDiffFileIds; + useCodeReview().reviewedIds = reviewedIds; }); - it('passes the viewedDiffFileIds to the FileTree', async () => { + it('passes the reviewedIds to the FileTree', async () => { createComponent(); await nextTick(); - expect(getFileRow().props('viewedFiles')).toBe(viewedDiffFileIds); + expect(getFileRow().props('viewedFiles')).toBe(reviewedIds); }); }); diff --git a/spec/frontend/diffs/stores/code_review_spec.js b/spec/frontend/diffs/stores/code_review_spec.js new file mode 100644 index 00000000000000..2aa7712ff01420 --- /dev/null +++ b/spec/frontend/diffs/stores/code_review_spec.js @@ -0,0 +1,107 @@ +import { createTestingPinia } from '@pinia/testing'; +import { useCodeReview } from '~/diffs/stores/code_review'; + +describe('CodeReview store', () => { + beforeEach(() => { + createTestingPinia({ stubActions: false }); + }); + + describe('#setMrPath', () => { + it('sets mr path', () => { + useCodeReview().setMrPath('foo'); + expect(useCodeReview().mrPath).toBe('foo'); + }); + }); + + describe('#restoreFromAutosave', () => { + it('restores from autosave', () => { + const items = ['bar']; + useCodeReview().setMrPath('foo'); + localStorage.setItem(useCodeReview().autosaveKey, JSON.stringify(items)); + useCodeReview().restoreFromAutosave(); + expect(useCodeReview().reviewedIds.bar).toBe(true); + }); + }); + + describe('#restoreFromLegacyMrReviews', () => { + it('restores from autosave', () => { + const items = { bar: ['baz'] }; + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify(items)); + useCodeReview().restoreFromLegacyMrReviews(); + expect(useCodeReview().reviewedIds.baz).toBe(true); + }); + + it('does not restore hash: values', () => { + const items = { bar: ['hash:baz'] }; + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify(items)); + useCodeReview().restoreFromLegacyMrReviews(); + expect(useCodeReview().reviewedIds.baz).toBe(undefined); + }); + + it('stores state', () => { + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify({ bar: ['baz'] })); + useCodeReview().restoreFromLegacyMrReviews(); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('baz'); + }); + }); + + describe('#setReviewed', () => { + it('marks file as reviewed', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', true); + expect(useCodeReview().reviewedIds.bar).toBe(true); + }); + + it('stores state', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', true); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('bar'); + }); + }); + + describe('#removeId', () => { + it('removes existing ID', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().reviewedIds = { bar: true }; + useCodeReview().removeId('bar'); + expect(useCodeReview().reviewedIds.bar).toBe(undefined); + }); + }); + + describe('#autosave', () => { + it('stores state', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().reviewedIds = { bar: true }; + useCodeReview().autosave(); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('bar'); + }); + + it('clears storage', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().autosave(); + expect(localStorage.getItem(useCodeReview().autosaveKey)).toBe(null); + }); + }); + + describe('#markedAsViewedIds', () => { + it('returns only viewed ids', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', false); + useCodeReview().setReviewed('baz', true); + expect(useCodeReview().markedAsViewedIds).toMatchObject(['baz']); + }); + }); + + describe('#autosaveKey', () => { + it('returns autosave key', () => { + useCodeReview().setMrPath('foo'); + expect(useCodeReview().autosaveKey).toBe('code-review-foo'); + }); + }); +}); diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index 3f01d6877c339e..f1d74a2b1c140e 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -113,10 +113,6 @@ describe('legacyDiffs actions', () => { const projectPath = '/root/project'; const dismissEndpoint = '/-/user_callouts'; const showSuggestPopover = false; - const mrReviews = { - a: ['z', 'a'], - b: ['y', 'a'], - }; const diffViewType = 'inline'; return testAction( @@ -130,7 +126,6 @@ describe('legacyDiffs actions', () => { projectPath, dismissEndpoint, showSuggestPopover, - mrReviews, diffViewType, }, { @@ -155,22 +150,9 @@ describe('legacyDiffs actions', () => { projectPath, dismissEndpoint, showSuggestPopover, - mrReviews, diffViewType, }, }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { codeReviewId: 'z', seen: true }, - }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { codeReviewId: 'a', seen: true }, - }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { codeReviewId: 'y', seen: true }, - }, ], [], ); @@ -2205,35 +2187,6 @@ describe('legacyDiffs actions', () => { ); }); - describe('reviewFile', () => { - const file = { - code_review_id: '123', - file_hash: 'xyz', - file_identifier_hash: 'abc', - load_collapsed_diff_url: 'gitlab-org/gitlab-test/-/merge_requests/1/diffs', - }; - it.each` - reviews | diffFile | reviewed - ${{ abc: ['123'] }} | ${file} | ${true} - ${{}} | ${file} | ${false} - `( - 'sets reviews ($reviews) to localStorage and state for file $file if it is marked reviewed=$reviewed', - ({ reviews, diffFile, reviewed }) => { - store.reviewFile({ - file: diffFile, - reviewed, - }); - - expect(localStorage.setItem).toHaveBeenCalledTimes(1); - expect(localStorage.setItem).toHaveBeenCalledWith( - 'code-review-gitlab-org/gitlab-test/-/merge_requests/1', - JSON.stringify(reviews), - ); - expect(store[types.SET_MR_FILE_REVIEWS]).toHaveBeenCalledWith(reviews); - }, - ); - }); - describe('toggleFileCommentForm', () => { it('commits TOGGLE_FILE_COMMENT_FORM', () => { const file = getDiffFileMock(); diff --git a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js index 0b61c9ae23b52b..c3954c03243fe2 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js @@ -990,22 +990,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('SET_DIFF_FILE_VIEWED', () => { - beforeEach(() => { - store.viewedDiffFileIds = { 123: true }; - }); - - it.each` - codeReviewId | bool | outcome - ${'abc'} | ${true} | ${{ 123: true, abc: true }} - ${'123'} | ${false} | ${{ 123: false }} - `('sets the viewed files list to $bool for the id $id', ({ codeReviewId, bool, outcome }) => { - store[types.SET_DIFF_FILE_VIEWED]({ codeReviewId, seen: bool }); - - expect(store.viewedDiffFileIds).toEqual(outcome); - }); - }); - describe('Set highlighted row', () => { it('sets highlighted row', () => { store[types.SET_HIGHLIGHTED_ROW]('ABC_123'); @@ -1270,21 +1254,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('SET_MR_FILE_REVIEWS', () => { - it.each` - newReviews | oldReviews - ${{ abc: ['123'] }} | ${{}} - ${{ abc: [] }} | ${{ abc: ['123'] }} - ${{}} | ${{ abc: ['123'] }} - `('sets mrReviews to $newReviews', ({ newReviews, oldReviews }) => { - store.$patch({ mrReviews: oldReviews }); - - store[types.SET_MR_FILE_REVIEWS](newReviews); - - expect(store.mrReviews).toStrictEqual(newReviews); - }); - }); - describe('TOGGLE_FILE_COMMENT_FORM', () => { it('toggles diff files hasCommentForm', () => { store.$patch({ diffFiles: [{ file_path: 'path', hasCommentForm: false }] }); diff --git a/spec/frontend/diffs/utils/file_reviews_spec.js b/spec/frontend/diffs/utils/file_reviews_spec.js index a54b802ff20a56..ccd27a5ae3e3e0 100644 --- a/spec/frontend/diffs/utils/file_reviews_spec.js +++ b/spec/frontend/diffs/utils/file_reviews_spec.js @@ -11,14 +11,14 @@ import { function getDefaultReviews() { return { - abc: ['123', '098'], + abc: ['123', 'hash:xyz', '098', 'hash:uvw'], }; } describe('File Review(s) utilities', () => { const mrPath = 'my/fake/mr/42'; - const storageKey = `code-review-${mrPath}`; - const file = { code_review_id: '123', file_hash: 'xyz', file_identifier_hash: 'abc' }; + const storageKey = `${mrPath}-file-reviews`; + const file = { id: '123', file_hash: 'xyz', file_identifier_hash: 'abc' }; const storedValue = JSON.stringify(getDefaultReviews()); let reviews; @@ -31,9 +31,9 @@ describe('File Review(s) utilities', () => { describe('isFileReviewed', () => { it.each` - description | diffFile | fileReviews - ${'the file does not have an `id`'} | ${{ ...file, code_review_id: undefined }} | ${getDefaultReviews()} - ${'there are no reviews for the file'} | ${file} | ${{ ...getDefaultReviews(), abc: undefined }} + description | diffFile | fileReviews + ${'the file does not have an `id`'} | ${{ ...file, id: undefined }} | ${getDefaultReviews()} + ${'there are no reviews for the file'} | ${file} | ${{ ...getDefaultReviews(), abc: undefined }} `('returns `false` if $description', ({ diffFile, fileReviews }) => { expect(isFileReviewed(fileReviews, diffFile)).toBe(false); }); @@ -44,8 +44,8 @@ describe('File Review(s) utilities', () => { }); describe('reviewStatuses', () => { - const file1 = { code_review_id: '123', hash: 'xyz', file_identifier_hash: 'abc' }; - const file2 = { code_review_id: '098', hash: 'uvw', file_identifier_hash: 'abc' }; + const file1 = { id: '123', hash: 'xyz', file_identifier_hash: 'abc' }; + const file2 = { id: '098', hash: 'uvw', file_identifier_hash: 'abc' }; it.each` mrReviews | files | fileReviews @@ -112,11 +112,11 @@ describe('File Review(s) utilities', () => { response | diffFile | description ${true} | ${file} | ${'has an `.id` and a `.file_identifier_hash`'} ${false} | ${{ file_identifier_hash: 'abc' }} | ${'does not have an `.id`'} - ${false} | ${{ ...file, code_review_id: undefined }} | ${'has an undefined `.id`'} - ${false} | ${{ ...file, code_review_id: null }} | ${'has a null `.id`'} - ${false} | ${{ ...file, code_review_id: 0 }} | ${'has an `.id` set to 0'} - ${false} | ${{ ...file, code_review_id: false }} | ${'has an `.id` set to false'} - ${false} | ${{ code_review_id: '123' }} | ${'does not have a `.file_identifier_hash`'} + ${false} | ${{ ...file, id: undefined }} | ${'has an undefined `.id`'} + ${false} | ${{ ...file, id: null }} | ${'has a null `.id`'} + ${false} | ${{ ...file, id: 0 }} | ${'has an `.id` set to 0'} + ${false} | ${{ ...file, id: false }} | ${'has an `.id` set to false'} + ${false} | ${{ id: '123' }} | ${'does not have a `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: undefined }} | ${'has an undefined `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: null }} | ${'has a null `.file_identifier_hash`'} ${false} | ${{ ...file, file_identifier_hash: 0 }} | ${'has a `.file_identifier_hash` set to 0'} @@ -128,7 +128,7 @@ describe('File Review(s) utilities', () => { describe('markFileReview', () => { it("adds a review when there's nothing that already exists", () => { - expect(markFileReview(null, file)).toStrictEqual({ abc: ['123'] }); + expect(markFileReview(null, file)).toStrictEqual({ abc: ['123', 'hash:xyz'] }); }); it("overwrites an existing review if it's for the same file (identifier hash)", () => { @@ -136,21 +136,21 @@ describe('File Review(s) utilities', () => { }); it('removes a review from the list when `reviewed` is `false`', () => { - expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098'] }); + expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098', 'hash:uvw'] }); }); it('adds a new review if the file ID is new', () => { - const updatedFile = { ...file, code_review_id: '098', file_hash: 'uvw' }; - const allReviews = markFileReview({ abc: ['123'] }, updatedFile); + const updatedFile = { ...file, id: '098', file_hash: 'uvw' }; + const allReviews = markFileReview({ abc: ['123', 'hash:xyz'] }, updatedFile); expect(allReviews).toStrictEqual(getDefaultReviews()); - expect(allReviews.abc).toStrictEqual(['123', '098']); + expect(allReviews.abc).toStrictEqual(['123', 'hash:xyz', '098', 'hash:uvw']); }); it.each` description | diffFile ${'missing an `.id`'} | ${{ file_identifier_hash: 'abc' }} - ${'missing a `.file_identifier_hash`'} | ${{ code_review_id: '123' }} + ${'missing a `.file_identifier_hash`'} | ${{ id: '123' }} `("doesn't modify the reviews if the file is $description", ({ diffFile }) => { expect(markFileReview(reviews, diffFile)).toStrictEqual(getDefaultReviews()); }); @@ -158,11 +158,7 @@ describe('File Review(s) utilities', () => { it('removes the file key if there are no more reviews for it', () => { let updated = markFileReview(reviews, file, false); - updated = markFileReview( - updated, - { ...file, code_review_id: '098', file_hash: 'uvw' }, - false, - ); + updated = markFileReview(updated, { ...file, id: '098', file_hash: 'uvw' }, false); expect(updated).toStrictEqual({}); }); -- GitLab