diff --git a/app/assets/javascripts/commits.js b/app/assets/javascripts/commits.js index 39dc4a4e9e57b5b5aa2cf10f962af37834126dde..8c73bb34cc62729cd129fbed9a85980eeb483ddd 100644 --- a/app/assets/javascripts/commits.js +++ b/app/assets/javascripts/commits.js @@ -1,9 +1,38 @@ import $ from 'jquery'; -import { n__ } from '~/locale'; +import { n__, s__ } from '~/locale'; +import { createAlert } from '~/alert'; +import createDefaultClient from '~/lib/graphql'; +import { sanitize } from '~/lib/dompurify'; +import { loadingIconForLegacyJS } from '~/loading_icon_for_legacy_js'; +import commitDetailsQuery from '~/projects/commits/graphql/queries/commit_details.query.graphql'; import axios from './lib/utils/axios_utils'; import { localTimeAgo } from './lib/utils/datetime_utility'; import Pager from './pager'; +const NEWLINE_CHAR = '
'; + +const defaultClient = createDefaultClient(); + +const handleError = () => + createAlert({ message: s__('Commits|Something went wrong while fetching commit details') }); + +const fetchCommitDetails = async (commitId) => { + const { projectFullPath: projectPath } = document.body.dataset; + let commit; + + try { + const { data } = await defaultClient.query({ + query: commitDetailsQuery, + variables: { projectPath, ref: commitId }, + }); + commit = data?.project?.repository?.commit || {}; + } catch (error) { + handleError(); + } + + return commit; +}; + export default class CommitsList { constructor(limit = 0) { this.timer = null; @@ -16,6 +45,7 @@ export default class CommitsList { this.searchField = $('#commits-search'); this.lastSearch = this.searchField.val(); this.initSearch(); + this.initCommitDetails(); } initSearch() { @@ -26,6 +56,32 @@ export default class CommitsList { }); } + initCommitDetails() { + if (!gon?.features?.loadCommitDetailsAsync) return; + this.content.on('click', '.js-toggle-button', ({ currentTarget }) => + this.handleToggleCommitDetails(currentTarget.dataset), + ); + } + + async handleToggleCommitDetails({ commitId }) { + const contentElement = this.content.find(`.js-toggle-content[data-commit-id="${commitId}"]`); + if (!contentElement || contentElement.data('content-loaded')) return; + contentElement.html(loadingIconForLegacyJS({ inline: true, size: 'sm' })); + + const commit = await fetchCommitDetails(commitId); + let descriptionHtml = commit?.descriptionHtml; + if (!descriptionHtml) { + handleError(); + return; + } + + if (descriptionHtml.startsWith(NEWLINE_CHAR)) + descriptionHtml = descriptionHtml.substring(NEWLINE_CHAR.length); // remove newline to avoid extra empty line before the description + + contentElement.html(sanitize(descriptionHtml)); + contentElement.attr('data-content-loaded', 'true'); + } + filterResults() { const form = $('.commits-search-form'); const search = this.searchField.val(); diff --git a/app/assets/javascripts/projects/commits/graphql/queries/commit_details.query.graphql b/app/assets/javascripts/projects/commits/graphql/queries/commit_details.query.graphql new file mode 100644 index 0000000000000000000000000000000000000000..582c9ae7327295f0dbb0cda10bbe87696340d175 --- /dev/null +++ b/app/assets/javascripts/projects/commits/graphql/queries/commit_details.query.graphql @@ -0,0 +1,11 @@ +query getCommitDetails($projectPath: ID!, $ref: String!) { + project(fullPath: $projectPath) { + id + repository { + commit(ref: $ref) { + id + descriptionHtml + } + } + } +} diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 14637437d0a3093540650d7be9b5067fc93fe260..3fb621af850f174c1e57dac6f93669d7e1810cf5 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -18,6 +18,9 @@ class Projects::CommitsController < Projects::ApplicationController before_action :validate_path, if: -> { !request.format.atom? } before_action :set_is_ambiguous_ref, only: [:show] before_action :set_commits, except: :commits_root + before_action do + push_frontend_feature_flag(:load_commit_details_async, @project) + end feature_category :source_code_management urgency :low, [:signatures, :show] diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 308308ccad5953f432b03a8474ddbe91900f5901..62a9ce3fc8cce12971b03459a1e4a5a2a4e23f99 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -17,6 +17,7 @@ - commit_status = commit.detailed_status_for(ref) - show_legacy_ci_icon = local_assigns.fetch(:show_legacy_ci_icon, true) - is_blob_page = local_assigns.fetch(:is_blob_page, false) +- is_commits_page = local_assigns.fetch(:is_commits_page, false) - tags = commit.tags_for_display - collapsible = local_assigns.fetch(:collapsible, true) - link_data_attrs = local_assigns.fetch(:link_data_attrs, {}) @@ -49,13 +50,14 @@ = commit.short_id - if commit.description? && collapsible = render Pajamas::ButtonComponent.new(icon: 'ellipsis_h', - button_options: { class: 'button-ellipsis-horizontal js-toggle-button', data: { toggle: 'tooltip', container: 'body', collapse_title: toggle_commit_message, expand_title: toggle_commit_message }, title: toggle_commit_message, aria: { label: toggle_commit_message }}) + button_options: { class: 'button-ellipsis-horizontal js-toggle-button', data: { toggle: 'tooltip', container: 'body', collapse_title: toggle_commit_message, expand_title: toggle_commit_message, commit_id: commit.id }, title: toggle_commit_message, aria: { label: toggle_commit_message }}) = render partial: 'projects/commits/committer', locals: { commit: commit } = render_if_exists 'projects/commits/project_namespace', show_project_name: show_project_name, project: project - if commit.description? - %pre{ class: ["commit-row-description gl-whitespace-pre-wrap", (collapsible ? "js-toggle-content" : "!gl-block")] } - = preserve(markdown_field(commit, :description)) + %pre{ class: ["commit-row-description gl-whitespace-pre-wrap", (collapsible ? "js-toggle-content" : "!gl-block")], data: { commit_id: commit.id } } + - if !Feature.enabled?(:load_commit_details_async, @project) || !is_commits_page + = preserve(markdown_field(commit, :description)) .commit-actions.gl-flex.gl-items-center.gl-gap-3 %div{ class: is_blob_page ? "gl-hidden sm:gl-flex gl-items-center gl-gap-3": "gl-flex gl-items-center gl-gap-3" } diff --git a/app/views/projects/commits/_commits.html.haml b/app/views/projects/commits/_commits.html.haml index 9df8fe6c1eda1eb60725382c70e03577c210f47b..3f9c7ff17e144551235c4e0475736805393fcd0a 100644 --- a/app/views/projects/commits/_commits.html.haml +++ b/app/views/projects/commits/_commits.html.haml @@ -2,6 +2,7 @@ - merge_request = local_assigns.fetch(:merge_request, nil) - project = local_assigns.fetch(:project) { merge_request&.project } - ref = local_assigns.fetch(:ref) { merge_request&.source_branch } +- is_commits_page = local_assigns.fetch(:is_commits_page, false) - can_update_merge_request = can?(current_user, :update_merge_request, @merge_request) - commits = @commits&.map { |commit| commit.present(current_user: current_user) } @@ -14,7 +15,7 @@ %li.gl-border-t{ data: { day: day } } %ul.content-list.commit-list.flex-list - = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request } + = render partial: 'projects/commits/commit', collection: daily_commits, locals: { project: project, ref: ref, merge_request: merge_request, is_commits_page: is_commits_page } - if context_commits.present? %li.js-commit-header.gl-mt-3.gl-py-2 @@ -25,7 +26,7 @@ %li.gl-border-t %ul.content-list.commit-list.flex-list - = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request } + = render partial: 'projects/commits/commit', collection: context_commits, locals: { project: project, ref: ref, merge_request: merge_request, is_commits_page: is_commits_page } - if hidden > 0 && !@merge_request %li diff --git a/app/views/projects/commits/show.html.haml b/app/views/projects/commits/show.html.haml index bfaaa6819777570891cee6506818b1b1a9726ece..8d16d9eb2e77f39837c9dc62a79de0df3434e37d 100644 --- a/app/views/projects/commits/show.html.haml +++ b/app/views/projects/commits/show.html.haml @@ -36,7 +36,7 @@ %div{ id: dom_id(@project) } %ol#commits-list.list-unstyled.content_list - = render 'commits', project: @project, ref: @ref + = render 'commits', project: @project, ref: @ref, is_commits_page: true = gl_loading_icon(size: 'lg', css_class: 'loading hide') -# https://gitlab.com/gitlab-org/gitlab/-/issues/408388#note_1578533983 diff --git a/config/feature_flags/wip/load_commit_details_async.yml b/config/feature_flags/wip/load_commit_details_async.yml new file mode 100644 index 0000000000000000000000000000000000000000..ce71f073e0b8f5d4d444cdabebf536ce52bad6f8 --- /dev/null +++ b/config/feature_flags/wip/load_commit_details_async.yml @@ -0,0 +1,9 @@ +--- +name: load_commit_details_async +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/515892 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183655 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/523619 +milestone: '17.10' +group: group::source code +type: wip +default_enabled: false diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1638db590f0775edcf98788dd9e0cabc0070eb13..49882aed3446ef0cb3cfae4b3c1de0e4beb1e4df 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14566,6 +14566,9 @@ msgstr "" msgid "Commits|No related merge requests found" msgstr "" +msgid "Commits|Something went wrong while fetching commit details" +msgstr "" + msgid "Committed by" msgstr "" diff --git a/spec/frontend/commits_spec.js b/spec/frontend/commits_spec.js index c79170aa37e6fc03f00311652539ac1f904654cb..171cb3651b77cadedea5edd781b584b3423a2a07 100644 --- a/spec/frontend/commits_spec.js +++ b/spec/frontend/commits_spec.js @@ -1,12 +1,28 @@ import MockAdapter from 'axios-mock-adapter'; import $ from 'jquery'; import 'vendor/jquery.endless-scroll'; +import waitForPromises from 'helpers/wait_for_promises'; import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import CommitsList from '~/commits'; import axios from '~/lib/utils/axios_utils'; import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; +import { sanitize } from '~/lib/dompurify'; import Pager from '~/pager'; +jest.mock('~/lib/dompurify', () => ({ + sanitize: jest.fn().mockImplementation((html) => html), +})); + +jest.mock('~/lib/graphql', () => { + return () => ({ + query: jest.fn().mockResolvedValue({ + data: { + project: { repository: { commit: { descriptionHtml: '
<p>Description HTML</p>' } } }, + }, + }), + }); +}); + describe('Commits List', () => { let commitsList; @@ -15,8 +31,11 @@ describe('Commits List', () => { <form class="commits-search-form" action="/h5bp/html5-boilerplate/commits/main"> <input id="commits-search"> </form> - <ol id="commits-list"></ol> - `); + <ol id="commits-list"> + <button class="js-toggle-button" data-commit-id="123">Toggle</button> + <div class="js-toggle-content" data-commit-id="123"></div> + </ol> + `); jest.spyOn(Pager, 'init').mockImplementation(() => {}); commitsList = new CommitsList(25); }); @@ -89,4 +108,69 @@ describe('Commits List', () => { expect(commitsList.lastSearch).toEqual(''); }); }); + + describe('commit details', () => { + const findContentToggleButton = () => document.querySelector('.js-toggle-button'); + const findLoadingSpinner = () => document.querySelector('.gl-spinner'); + const findContentElement = () => document.querySelector('.js-toggle-content'); + let jqueryOnSpy; + + beforeEach(() => { + gon.features = { loadCommitDetailsAsync: true }; + jqueryOnSpy = jest.spyOn($.fn, 'on'); + commitsList = new CommitsList(); + }); + + it('initializes click handler when feature flag is enabled', () => { + commitsList = new CommitsList(); + + expect(jqueryOnSpy).toHaveBeenCalledWith('click', '.js-toggle-button', expect.any(Function)); + }); + + it('does not initialize click handler when feature flag is disabled', () => { + jqueryOnSpy.mockClear(); + gon.features = { loadCommitDetailsAsync: false }; + commitsList = new CommitsList(); + + expect(jqueryOnSpy).not.toHaveBeenCalledWith( + 'click', + '.js-toggle-button', + expect.any(Function), + ); + }); + + describe('when clicking toggle button', () => { + let contentElement; + + beforeEach(() => { + contentElement = findContentElement(); + }); + + it('fetches and renders commit details', async () => { + findContentToggleButton().click(); + + expect(findLoadingSpinner()).not.toBe(null); + + await waitForPromises(); + expect(contentElement.innerHTML).toBe('<p>Description HTML</p>'); + expect(findLoadingSpinner()).toBe(null); + }); + + it('sanitizes the content', async () => { + findContentToggleButton().click(); + + await waitForPromises(); + expect(sanitize).toHaveBeenCalledWith('<p>Description HTML</p>'); + }); + + it('sets content loaded attribute when details are fetched', async () => { + expect(contentElement.dataset.contentLoaded).toBeUndefined(); + findContentToggleButton().click(); + + await waitForPromises(); + expect(contentElement.dataset.contentLoaded).toBe('true'); + expect(findLoadingSpinner()).toBe(null); + }); + }); + }); });