diff --git a/app/assets/javascripts/batch_comments/components/review_bar.vue b/app/assets/javascripts/batch_comments/components/review_bar.vue index 3cd1a2525e9a72320f6f5a1b74e469719aad7d4a..111b670596b98b866badbf608bc1d2f2249993fa 100644 --- a/app/assets/javascripts/batch_comments/components/review_bar.vue +++ b/app/assets/javascripts/batch_comments/components/review_bar.vue @@ -2,10 +2,20 @@ import { mapActions, mapGetters } from 'vuex'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { REVIEW_BAR_VISIBLE_CLASS_NAME } from '../constants'; +import { PREVENT_LEAVING_PENDING_REVIEW } from '../i18n'; import PreviewDropdown from './preview_dropdown.vue'; import PublishButton from './publish_button.vue'; import SubmitDropdown from './submit_dropdown.vue'; +function closeInterrupt(event) { + event.preventDefault(); + + // This is the correct way to write backwards-compatible beforeunload listeners + // https://developer.chrome.com/blog/page-lifecycle-api/#the-beforeunload-event + /* eslint-disable-next-line no-return-assign, no-param-reassign */ + return (event.returnValue = PREVENT_LEAVING_PENDING_REVIEW); +} + export default { components: { PreviewDropdown, @@ -25,8 +35,26 @@ export default { }, mounted() { document.body.classList.add(REVIEW_BAR_VISIBLE_CLASS_NAME); + /* + * This stuff is a lot trickier than it looks. + * + * Mandatory reading: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event + * Some notable sentences: + * - "[...] browsers may not display prompts created in beforeunload event handlers unless the + * page has been interacted with, or may even not display them at all." + * - "Especially on mobile, the beforeunload event is not reliably fired." + * - "The beforeunload event is not compatible with the back/forward cache (bfcache) [...] + * It is recommended that developers listen for beforeunload only in this scenario, and only + * when they actually have unsaved changes, so as to minimize the effect on performance." + * + * Please ensure that this is really not working before you modify it, because there are a LOT + * of scenarios where browser behavior will make it _seem_ like it's not working, but it actually + * is under the right combination of contexts. + */ + window.addEventListener('beforeunload', closeInterrupt, { capture: true }); }, beforeDestroy() { + window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); document.body.classList.remove(REVIEW_BAR_VISIBLE_CLASS_NAME); }, methods: { diff --git a/app/assets/javascripts/batch_comments/i18n.js b/app/assets/javascripts/batch_comments/i18n.js new file mode 100644 index 0000000000000000000000000000000000000000..6cdbf00f9cadb30651dfa758df4e820557a3edde --- /dev/null +++ b/app/assets/javascripts/batch_comments/i18n.js @@ -0,0 +1,3 @@ +import { __ } from '~/locale'; + +export const PREVENT_LEAVING_PENDING_REVIEW = __('There are unsubmitted review comments.'); diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js index a44b9827fe9b57ba76f7922303b5e5ba714bf319..863d2a99972214c313fbd22b73783c46415ef164 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js @@ -1,9 +1,12 @@ import { isEmpty } from 'lodash'; + import createFlash from '~/flash'; import { scrollToElement } from '~/lib/utils/common_utils'; import { __ } from '~/locale'; + import { CHANGES_TAB, DISCUSSION_TAB, SHOW_TAB } from '../../../constants'; import service from '../../../services/drafts_service'; + import * as types from './mutation_types'; export const saveDraft = ({ dispatch }, draft) => @@ -15,6 +18,7 @@ export const addDraftToDiscussion = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + return res; }) .catch(() => { @@ -29,6 +33,7 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + return res; }) .catch(() => { diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 6c0c9c4e1d07f75b84a528fd6422db47aee67748..1cc96ef3d54b80c94da4ca0929b49e0292209b80 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -71,12 +71,15 @@ export const DIFF_FILE_MANUAL_COLLAPSE = 'manual'; export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; export const STATE_ERRORED = 'errored'; +export const STATE_PENDING_REVIEW = 'pending_comments'; // State machine transitions export const TRANSITION_LOAD_START = 'LOAD_START'; export const TRANSITION_LOAD_ERROR = 'LOAD_ERROR'; export const TRANSITION_LOAD_SUCCEED = 'LOAD_SUCCEED'; export const TRANSITION_ACKNOWLEDGE_ERROR = 'ACKNOWLEDGE_ERROR'; +export const TRANSITION_HAS_PENDING_REVIEW = 'PENDING_REVIEW'; +export const TRANSITION_NO_REVIEW = 'NO_REVIEW'; export const RENAMED_DIFF_TRANSITIONS = { [`${STATE_IDLING}:${TRANSITION_LOAD_START}`]: STATE_LOADING, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2a80ac4969427e70ecedc3a3f77e2208998ef5f3..2ba70cde3f6b141860bb40c05546383409c4195b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39296,6 +39296,9 @@ msgstr "" msgid "There are several size limits in place." msgstr "" +msgid "There are unsubmitted review comments." +msgstr "" + msgid "There is already a repository with that name on disk" msgstr "" diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index fafaea8ac6853fea2ea5d54bad979f754f33cb4f..f892b01e6247f37a68a4ee508df3a321fca42e49 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -101,7 +101,7 @@ write_diff_comment - visit_overview + visit_overview_with_pending_comment end it 'can add comment to review' do @@ -232,6 +232,14 @@ def visit_overview wait_for_requests end + def visit_overview_with_pending_comment + accept_alert do + visit project_merge_request_path(merge_request.project, merge_request) + end + + wait_for_requests + end + def write_diff_comment(**params) click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']")) diff --git a/spec/frontend/batch_comments/components/review_bar_spec.js b/spec/frontend/batch_comments/components/review_bar_spec.js index f50db6ab210bb84f98323af375071bba4857165c..f98e0a4c64a35e2c47f3fb65538d562f573c1f45 100644 --- a/spec/frontend/batch_comments/components/review_bar_spec.js +++ b/spec/frontend/batch_comments/components/review_bar_spec.js @@ -6,6 +6,8 @@ import createStore from '../create_batch_comments_store'; describe('Batch comments review bar component', () => { let store; let wrapper; + let addEventListenerSpy; + let removeEventListenerSpy; const createComponent = (propsData = {}) => { store = createStore(); @@ -18,25 +20,58 @@ describe('Batch comments review bar component', () => { beforeEach(() => { document.body.className = ''; + + addEventListenerSpy = jest.spyOn(window, 'addEventListener'); + removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); }); afterEach(() => { + addEventListenerSpy.mockRestore(); + removeEventListenerSpy.mockRestore(); wrapper.destroy(); }); - it('it adds review-bar-visible class to body when review bar is mounted', async () => { - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + describe('when mounted', () => { + it('it adds review-bar-visible class to body', async () => { + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + + createComponent(); + + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + }); - createComponent(); + it('it adds a blocking handler to the `beforeunload` window event', () => { + expect(addEventListenerSpy).not.toBeCalled(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + createComponent(); + + expect(addEventListenerSpy).toHaveBeenCalledTimes(1); + expect(addEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); + }); }); - it('it removes review-bar-visible class to body when review bar is destroyed', async () => { - createComponent(); + describe('before destroyed', () => { + it('it removes review-bar-visible class to body', async () => { + createComponent(); - wrapper.destroy(); + wrapper.destroy(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + }); + + it('it removes the blocking handler from the `beforeunload` window event', () => { + createComponent(); + + expect(removeEventListenerSpy).not.toBeCalled(); + + wrapper.destroy(); + + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); + }); }); });