From 60415cd63b62335b67aec2319ec3c04b1ae0dba6 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Wed, 14 May 2025 22:44:22 +0400 Subject: [PATCH 1/8] Add image diff viewer --- .../javascripts/rapid_diffs/adapters.js | 2 + .../rapid_diffs/image_viewer/adapter.js | 24 +++++++++ .../components/rapid_diffs/_index.scss | 1 + .../components/rapid_diffs/image_viewer.scss | 14 +++++ .../rapid_diffs/diff_file_component.rb | 2 + .../viewers/image_view_component.html.haml | 4 ++ .../viewers/image_view_component.rb | 51 +++++++++++++++++++ lib/gitlab/diff/file.rb | 6 +++ locale/gitlab.pot | 3 ++ 9 files changed, 107 insertions(+) create mode 100644 app/assets/javascripts/rapid_diffs/image_viewer/adapter.js create mode 100644 app/assets/stylesheets/components/rapid_diffs/image_viewer.scss create mode 100644 app/components/rapid_diffs/viewers/image_view_component.html.haml create mode 100644 app/components/rapid_diffs/viewers/image_view_component.rb diff --git a/app/assets/javascripts/rapid_diffs/adapters.js b/app/assets/javascripts/rapid_diffs/adapters.js index bd76be708a2b23..1911f55bcc49eb 100644 --- a/app/assets/javascripts/rapid_diffs/adapters.js +++ b/app/assets/javascripts/rapid_diffs/adapters.js @@ -2,11 +2,13 @@ import { ExpandLinesAdapter } from '~/rapid_diffs/expand_lines/adapter'; import { OptionsMenuAdapter } from '~/rapid_diffs/options_menu/adapter'; import { ToggleFileAdapter } from '~/rapid_diffs/toggle_file/adapter'; import { DisableDiffSideAdapter } from '~/rapid_diffs/disable_diff_side/adapter'; +import { ImageAdapter } from '~/rapid_diffs/image_viewer/adapter'; const HEADER_ADAPTERS = [OptionsMenuAdapter, ToggleFileAdapter]; export const VIEWER_ADAPTERS = { text_inline: [...HEADER_ADAPTERS, ExpandLinesAdapter], text_parallel: [...HEADER_ADAPTERS, ExpandLinesAdapter, DisableDiffSideAdapter], + image: [...HEADER_ADAPTERS, ImageAdapter], no_preview: HEADER_ADAPTERS, }; diff --git a/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js new file mode 100644 index 00000000000000..8468b612d3f089 --- /dev/null +++ b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js @@ -0,0 +1,24 @@ +import Vue from 'vue'; +import ImageDiffViewer from '~/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue'; +import { MOUNTED } from '../events'; + +export const ImageAdapter = { + [MOUNTED]() { + const data = JSON.parse(this.diffElement.querySelector('[data-image-data]').dataset.imageData); + // eslint-disable-next-line no-new + new Vue({ + el: this.diffElement.querySelector('[data-image-view]'), + render(h) { + return h(ImageDiffViewer, { + props: { + oldPath: data.old_path || '', + newPath: data.new_path || '', + oldSize: data.old_size || '', + newSize: data.new_size || '', + diffMode: data.diff_mode, + }, + }); + }, + }); + }, +}; diff --git a/app/assets/stylesheets/components/rapid_diffs/_index.scss b/app/assets/stylesheets/components/rapid_diffs/_index.scss index ba7eb003d18383..f3b639269a2d83 100644 --- a/app/assets/stylesheets/components/rapid_diffs/_index.scss +++ b/app/assets/stylesheets/components/rapid_diffs/_index.scss @@ -3,3 +3,4 @@ @import './text_file_viewers'; @import './no_preview'; @import './empty_state'; +@import './image_viewer'; diff --git a/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss b/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss new file mode 100644 index 00000000000000..15de494f015de0 --- /dev/null +++ b/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss @@ -0,0 +1,14 @@ +.rd-image-diff { + margin: 0; +} + +.rd-image-diff-fallback { + @apply gl-p-5; + text-align: center; +} + +.rd-image-diff .diff-file-container, +.rd-image-diff .diff-viewer, +.rd-image-diff .image { + border-radius: inherit; +} diff --git a/app/components/rapid_diffs/diff_file_component.rb b/app/components/rapid_diffs/diff_file_component.rb index 72c4ee913df79a..837a9abd16825d 100644 --- a/app/components/rapid_diffs/diff_file_component.rb +++ b/app/components/rapid_diffs/diff_file_component.rb @@ -34,6 +34,8 @@ def viewer_component return Viewers::Text::InlineViewComponent end + return Viewers::ImageViewComponent if @diff_file.image_diff? + Viewers::NoPreviewComponent end diff --git a/app/components/rapid_diffs/viewers/image_view_component.html.haml b/app/components/rapid_diffs/viewers/image_view_component.html.haml new file mode 100644 index 00000000000000..e70508f0e6e870 --- /dev/null +++ b/app/components/rapid_diffs/viewers/image_view_component.html.haml @@ -0,0 +1,4 @@ +-# .diff-file class used for compatibility with legacy styles for now +.rd-image-diff.diff-file{ data: { image_data: image_data.to_json } } + .rd-image-diff-fallback{ data: { image_view: true } } + = s_('RapidDiffs|Loading image diff...') diff --git a/app/components/rapid_diffs/viewers/image_view_component.rb b/app/components/rapid_diffs/viewers/image_view_component.rb new file mode 100644 index 00000000000000..8e62c1f36662e5 --- /dev/null +++ b/app/components/rapid_diffs/viewers/image_view_component.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module RapidDiffs + module Viewers + class ImageViewComponent < ViewerComponent + include TreeHelper + + def self.viewer_name + 'image' + end + + def image_data + { + old_path: diff_file_old_blob_raw_url, + new_path: diff_file_blob_raw_url, + old_size: @diff_file.old_blob&.size, + new_size: @diff_file.new_blob&.size, + diff_mode: diff_mode + } + end + + def diff_file_old_blob_raw_url + sha = @diff_file.old_content_sha + return unless sha + + project_raw_url( + @diff_file.repository.project, + tree_join(@diff_file.old_content_sha, @diff_file.old_path), + only_path: true + ) + end + + def diff_file_blob_raw_url + project_raw_url( + @diff_file.repository.project, + tree_join(@diff_file.content_sha, @diff_file.file_path), + only_path: true + ) + end + + def diff_mode + return 'new' if @diff_file.new_file? + return 'deleted' if @diff_file.deleted_file? + return 'renamed' if @diff_file.renamed_file? + return 'mode_changed' if @diff_file.mode_changed? + + 'replaced' + end + end + end +end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 46ef6d2b102c4b..aa331f5cd68228 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -462,6 +462,12 @@ def whitespace_only? !collapsed? && diff_lines_for_serializer.nil? && (added_lines != 0 || removed_lines != 0) end + def image_diff? + return false if different_type? || external_storage_error? + + DiffViewer::Image.can_render?(self, verify_binary: !stored_externally?) + end + private def diffable_by_attribute? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6b7ec2c5c142b0..8926d00cca2cc7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50187,6 +50187,9 @@ msgstr "" msgid "RapidDiffs|Line %d" msgstr "" +msgid "RapidDiffs|Loading image diff..." +msgstr "" + msgid "RapidDiffs|Original line" msgstr "" -- GitLab From e83e13c67f5bc2a52399ffe6e734f48ad8d2d222 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Fri, 16 May 2025 12:39:24 +0400 Subject: [PATCH 2/8] Fix missing background in image diffs --- app/assets/stylesheets/framework/diffs.scss | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index cf727bc2144f9c..f234f2e490cc84 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -339,12 +339,10 @@ $diff-file-header: 41px; .diff-file-container { .frame.deleted { border: 1px solid var(--gl-status-danger-icon-color); - background-color: inherit; } .frame.added { border: 1px solid var(--gl-status-success-icon-color); - background-color: inherit; } .swipe.view, -- GitLab From a3d726fff0faa0930d83310e523bca0d467b0ea5 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Fri, 16 May 2025 13:19:02 +0400 Subject: [PATCH 3/8] Add adapter spec --- .../rapid_diffs/image_viewer/adapter.js | 4 +- .../rapid_diffs/image_viewer/adapter.spec.js | 67 +++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 spec/frontend/rapid_diffs/image_viewer/adapter.spec.js diff --git a/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js index 8468b612d3f089..7e702581ad45de 100644 --- a/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js +++ b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js @@ -13,8 +13,8 @@ export const ImageAdapter = { props: { oldPath: data.old_path || '', newPath: data.new_path || '', - oldSize: data.old_size || '', - newSize: data.new_size || '', + oldSize: data.old_size ? parseInt(data.old_size, 10) : undefined, + newSize: data.new_size ? parseInt(data.new_size, 10) : undefined, diffMode: data.diff_mode, }, }); diff --git a/spec/frontend/rapid_diffs/image_viewer/adapter.spec.js b/spec/frontend/rapid_diffs/image_viewer/adapter.spec.js new file mode 100644 index 00000000000000..4133f233b710de --- /dev/null +++ b/spec/frontend/rapid_diffs/image_viewer/adapter.spec.js @@ -0,0 +1,67 @@ +import { ImageAdapter } from '~/rapid_diffs/image_viewer/adapter'; +import { setHTMLFixture } from 'helpers/fixtures'; +import { DiffFile } from '~/rapid_diffs/diff_file'; + +jest.mock('~/alert'); +jest.mock('~/rapid_diffs/expand_lines/get_lines'); +jest.mock('~/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue', () => ({ + props: jest.requireActual('~/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue') + .default.props, + render(h) { + return h('div', { + attrs: { + 'data-image-diff-viewer': true, + 'data-old-path': this.oldPath, + 'data-new-path': this.newPath, + 'data-old-size': this.oldSize, + 'data-new-size': this.newSize, + 'data-diff-mode': this.diffMode, + }, + }); + }, +})); + +describe('ImageAdapter', () => { + const imageData = { + old_path: '/old', + new_path: '/old', + old_size: '10', + new_size: '20', + diff_mode: 'replaced', + }; + + const getDiffFile = () => document.querySelector('diff-file'); + const getDiffViewerApp = () => document.querySelector('[data-image-diff-viewer]'); + + const mount = () => { + setHTMLFixture(` + +
+
+
+
+
+ + `); + getDiffFile().mount({ + adapterConfig: { image: [ImageAdapter] }, + appData: {}, + unobserve: jest.fn(), + }); + }; + + beforeAll(() => { + customElements.define('diff-file', DiffFile); + }); + + it('shows image diff', () => { + mount(); + expect(getDiffViewerApp().dataset).toMatchObject({ + oldPath: imageData.old_path, + newPath: imageData.new_path, + oldSize: imageData.old_size, + newSize: imageData.new_size, + diffMode: imageData.diff_mode, + }); + }); +}); -- GitLab From c0a736161b411644b90c5d86748b3c0ccaebff3b Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Fri, 16 May 2025 13:29:18 +0400 Subject: [PATCH 4/8] Fix styles --- .../stylesheets/components/rapid_diffs/image_viewer.scss | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss b/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss index 15de494f015de0..5011466fadc7b5 100644 --- a/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss +++ b/app/assets/stylesheets/components/rapid_diffs/image_viewer.scss @@ -1,14 +1,9 @@ .rd-image-diff { margin: 0; + overflow: hidden; } .rd-image-diff-fallback { @apply gl-p-5; text-align: center; } - -.rd-image-diff .diff-file-container, -.rd-image-diff .diff-viewer, -.rd-image-diff .image { - border-radius: inherit; -} -- GitLab From 9cefe985806a8c9da9a57a576db0ff102e825b9f Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Fri, 16 May 2025 13:35:51 +0400 Subject: [PATCH 5/8] Add more specs --- spec/components/rapid_diffs/shared.rb | 11 +++++ .../viewers/image_view_component_spec.rb | 41 +++++++++++++++++++ spec/lib/gitlab/diff/file_spec.rb | 10 +++++ 3 files changed, 62 insertions(+) create mode 100644 spec/components/rapid_diffs/viewers/image_view_component_spec.rb diff --git a/spec/components/rapid_diffs/shared.rb b/spec/components/rapid_diffs/shared.rb index ac98d83690df63..a50fc14a2aad36 100644 --- a/spec/components/rapid_diffs/shared.rb +++ b/spec/components/rapid_diffs/shared.rb @@ -70,6 +70,17 @@ def render_component end end + context "when is image diff" do + before do + allow(diff_file).to receive_messages(diffable_text?: false, image_diff?: true) + end + + it "renders image viewer" do + render_component + expect(file_data['viewer']).to eq('image') + end + end + context "when no viewer found" do before do allow(diff_file).to receive_messages(text?: false, content_changed?: false) diff --git a/spec/components/rapid_diffs/viewers/image_view_component_spec.rb b/spec/components/rapid_diffs/viewers/image_view_component_spec.rb new file mode 100644 index 00000000000000..16e8e4e152c3ee --- /dev/null +++ b/spec/components/rapid_diffs/viewers/image_view_component_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe RapidDiffs::Viewers::ImageViewComponent, type: :component, feature_category: :code_review_workflow do + let_it_be(:diff_file) { build(:diff_file) } + let(:viewer) { page.find('[data-image-data]') } + + before do + allow(diff_file).to receive_message_chain(:old_blob, :size).and_return('200') + allow(diff_file).to receive_message_chain(:new_blob, :size).and_return('300') + end + + it 'provides image diff data' do + render_component + project = diff_file.repository.container + namespace = project.namespace + old_path = "/#{namespace.to_param}/#{project.to_param}/-/raw/#{diff_file.old_content_sha}/#{diff_file.file_path}" + new_path = "/#{namespace.to_param}/#{project.to_param}/-/raw/#{diff_file.content_sha}/#{diff_file.file_path}" + expect(image_data).to eq({ + "old_path" => old_path, + "new_path" => new_path, + "old_size" => '200', + "new_size" => '300', + "diff_mode" => 'replaced' + }) + end + + it 'renders image app mount element' do + render_component + expect(page).to have_css('[data-image-view]') + end + + def image_data + Gitlab::Json.parse(viewer['data-image-data']) + end + + def render_component(**args) + render_inline(described_class.new(diff_file: diff_file, **args)) + end +end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 13e9ccdc53f606..d77a7705bf14db 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -1312,6 +1312,16 @@ def popen(cmd, path=nil) end end + describe '#image_diff?' do + subject(:image_diff?) { diff_file.image_diff? } + + it 'returns true for image diffs' do + allow(diff_file).to receive_messages(different_type?: false, external_storage_error?: false) + allow(DiffViewer::Image).to receive(:can_render?).and_return(true) + expect(image_diff?).to eq(true) + end + end + describe '#modified_file?' do subject(:modified_file?) { diff_file.modified_file? } -- GitLab From 5c6ac0947981f5faa371d43dea225cc4e1c13460 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Tue, 20 May 2025 15:12:38 +0400 Subject: [PATCH 6/8] Don't encode already encoded image paths --- .../javascripts/rapid_diffs/image_viewer/adapter.js | 2 ++ .../components/content_viewer/viewers/image_viewer.vue | 6 ++++++ .../diff_viewer/viewers/image_diff/onion_skin_viewer.vue | 7 +++++++ .../diff_viewer/viewers/image_diff/swipe_viewer.vue | 7 +++++++ .../diff_viewer/viewers/image_diff/two_up_viewer.vue | 7 +++++++ .../components/diff_viewer/viewers/image_diff_viewer.vue | 6 ++++++ spec/frontend/rapid_diffs/image_viewer/adapter.spec.js | 4 ++-- .../content_viewer/viewers/image_viewer_spec.js | 9 +++++++++ .../diff_viewer/viewers/image_diff_viewer_spec.js | 6 ++++++ 9 files changed, 52 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js index 7e702581ad45de..9186268237221e 100644 --- a/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js +++ b/app/assets/javascripts/rapid_diffs/image_viewer/adapter.js @@ -16,6 +16,8 @@ export const ImageAdapter = { oldSize: data.old_size ? parseInt(data.old_size, 10) : undefined, newSize: data.new_size ? parseInt(data.new_size, 10) : undefined, diffMode: data.diff_mode, + // URLs are already encoded on the backend + encodePath: false, }, }); }, diff --git a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue index 9742118cd5f77a..cb126352a49295 100644 --- a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue @@ -26,6 +26,11 @@ export default { required: false, default: '', }, + encodePath: { + type: Boolean, + required: false, + default: true, + }, }, data() { return { @@ -47,6 +52,7 @@ export default { return this.width && this.height; }, safePath() { + if (!this.encodePath) return this.path; return this.path.startsWith(BLOB_PREFIX) ? this.path : encodeSaferUrl(this.path); }, }, diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue index a3d9b0ace341a5..2a12b372c0e4c2 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue @@ -15,6 +15,11 @@ export default { type: String, required: true, }, + encodePath: { + type: Boolean, + required: false, + default: true, + }, }, data() { return { @@ -123,6 +128,7 @@ export default { key="onionOldImg" :render-info="false" :path="oldPath" + :encode-path="encodePath" @imgLoaded="onionOldImgLoaded" /> @@ -139,6 +145,7 @@ export default { key="onionNewImg" :render-info="false" :path="newPath" + :encode-path="encodePath" @imgLoaded="onionNewImgLoaded" >