Merge branch 'tor/feature/actual-jump-to-next-diffs' into 'master'

Update Diff app so that Jump To Next Unresolved Thread tracks progress through page

See merge request gitlab-org/gitlab!71508

app/assets/javascripts/diffs/components/app.vue

@@ -44,6 +44,7 @@ import {
   TRACKING_MULTIPLE_FILES_MODE,
 } from '../constants';
 
+import { discussionIntersectionObserverHandlerFactory } from '../utils/discussions';
 import diffsEventHub from '../event_hub';
 import { reviewStatuses } from '../utils/file_reviews';
 import { diffsApp } from '../utils/performance';
@@ -86,6 +87,9 @@ export default {
     ALERT_MERGE_CONFLICT,
     ALERT_COLLAPSED_FILES,
   },
+  provide: {
+    discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+  },
   props: {
     endpoint: {
       type: String,

app/assets/javascripts/diffs/utils/discussions.js

+function normalize(processable) {
+  const { entry } = processable;
+  const offset = entry.rootBounds.bottom - entry.boundingClientRect.top;
+  const direction =
+    offset < 0 ? 'Up' : 'Down'; /* eslint-disable-line @gitlab/require-i18n-strings */
+
+  return {
+    ...processable,
+    entry: {
+      time: entry.time,
+      type: entry.isIntersecting ? 'intersection' : `scroll${direction}`,
+    },
+  };
+}
+
+function sort({ entry: alpha }, { entry: beta }) {
+  const diff = alpha.time - beta.time;
+  let order = 0;
+
+  if (diff < 0) {
+    order = -1;
+  } else if (diff > 0) {
+    order = 1;
+  } else if (alpha.type === 'intersection' && beta.type === 'scrollUp') {
+    order = 2;
+  } else if (alpha.type === 'scrollUp' && beta.type === 'intersection') {
+    order = -2;
+  }
+
+  return order;
+}
+
+function filter(entry) {
+  return entry.type !== 'scrollDown';
+}
+
+export function discussionIntersectionObserverHandlerFactory() {
+  let unprocessed = [];
+  let timer = null;
+
+  return (processable) => {
+    unprocessed.push(processable);
+
+    if (timer) {
+      clearTimeout(timer);
+    }
+
+    timer = setTimeout(() => {
+      unprocessed
+        .map(normalize)
+        .filter(filter)
+        .sort(sort)
+        .forEach((discussionObservationContainer) => {
+          const {
+            entry: { type },
+            currentDiscussion,
+            isFirstUnresolved,
+            isDiffsPage,
+            functions: { setCurrentDiscussionId, getPreviousUnresolvedDiscussionId },
+          } = discussionObservationContainer;
+
+          if (type === 'intersection') {
+            setCurrentDiscussionId(currentDiscussion.id);
+          } else if (type === 'scrollUp') {
+            setCurrentDiscussionId(
+              isFirstUnresolved
+                ? null
+                : getPreviousUnresolvedDiscussionId(currentDiscussion.id, isDiffsPage),
+            );
+          }
+        });
+
+      unprocessed = [];
+    }, 0);
+  };
+}

app/assets/javascripts/notes/components/discussion_notes.vue

 <script>
 import { mapGetters, mapActions } from 'vuex';
+import { GlIntersectionObserver } from '@gitlab/ui';
 import { __ } from '~/locale';
 import PlaceholderNote from '~/vue_shared/components/notes/placeholder_note.vue';
 import PlaceholderSystemNote from '~/vue_shared/components/notes/placeholder_system_note.vue';
@@ -16,7 +17,9 @@ export default {
     ToggleRepliesWidget,
     NoteEditedText,
     DiscussionNotesRepliesWrapper,
+    GlIntersectionObserver,
   },
+  inject: ['discussionObserverHandler'],
   props: {
     discussion: {
       type: Object,
@@ -54,7 +57,11 @@ export default {
     },
   },
   computed: {
-    ...mapGetters(['userCanReply']),
+    ...mapGetters([
+      'userCanReply',
+      'previousUnresolvedDiscussionId',
+      'firstUnresolvedDiscussionId',
+    ]),
     hasReplies() {
       return Boolean(this.replies.length);
     },
@@ -77,9 +84,20 @@ export default {
         url: this.discussion.discussion_path,
       };
     },
+    isFirstUnresolved() {
+      return this.firstUnresolvedDiscussionId === this.discussion.id;
+    },
+  },
+  observerOptions: {
+    threshold: 0,
+    rootMargin: '0px 0px -50% 0px',
   },
   methods: {
-    ...mapActions(['toggleDiscussion', 'setSelectedCommentPositionHover']),
+    ...mapActions([
+      'toggleDiscussion',
+      'setSelectedCommentPositionHover',
+      'setCurrentDiscussionId',
+    ]),
     componentName(note) {
       if (note.isPlaceholderNote) {
         if (note.placeholderType === SYSTEM_NOTE) {
@@ -110,6 +128,18 @@ export default {
         this.setSelectedCommentPositionHover();
       }
     },
+    observerTriggered(entry) {
+      this.discussionObserverHandler({
+        entry,
+        isFirstUnresolved: this.isFirstUnresolved,
+        currentDiscussion: { ...this.discussion },
+        isDiffsPage: !this.isOverviewTab,
+        functions: {
+          setCurrentDiscussionId: this.setCurrentDiscussionId,
+          getPreviousUnresolvedDiscussionId: this.previousUnresolvedDiscussionId,
+        },
+      });
+    },
   },
 };
 </script>
@@ -122,33 +152,35 @@ export default {
       @mouseleave="handleMouseLeave(discussion)"
     >
       <template v-if="shouldGroupReplies">
-        <component
-          :is="componentName(firstNote)"
-          :note="componentData(firstNote)"
-          :line="line || diffLine"
-          :discussion-file="discussion.diff_file"
-          :commit="commit"
-          :help-page-path="helpPagePath"
-          :show-reply-button="userCanReply"
-          :discussion-root="true"
-          :discussion-resolve-path="discussion.resolve_path"
-          :is-overview-tab="isOverviewTab"
-          @handleDeleteNote="$emit('deleteNote')"
-          @startReplying="$emit('startReplying')"
-        >
-          <template #discussion-resolved-text>
-            <note-edited-text
-              v-if="discussion.resolved"
-              :edited-at="discussion.resolved_at"
-              :edited-by="discussion.resolved_by"
-              :action-text="resolvedText"
-              class-name="discussion-headline-light js-discussion-headline discussion-resolved-text"
-            />
-          </template>
-          <template #avatar-badge>
-            <slot name="avatar-badge"></slot>
-          </template>
-        </component>
+        <gl-intersection-observer :options="$options.observerOptions" @update="observerTriggered">
+          <component
+            :is="componentName(firstNote)"
+            :note="componentData(firstNote)"
+            :line="line || diffLine"
+            :discussion-file="discussion.diff_file"
+            :commit="commit"
+            :help-page-path="helpPagePath"
+            :show-reply-button="userCanReply"
+            :discussion-root="true"
+            :discussion-resolve-path="discussion.resolve_path"
+            :is-overview-tab="isOverviewTab"
+            @handleDeleteNote="$emit('deleteNote')"
+            @startReplying="$emit('startReplying')"
+          >
+            <template #discussion-resolved-text>
+              <note-edited-text
+                v-if="discussion.resolved"
+                :edited-at="discussion.resolved_at"
+                :edited-by="discussion.resolved_by"
+                :action-text="resolvedText"
+                class-name="discussion-headline-light js-discussion-headline discussion-resolved-text"
+              />
+            </template>
+            <template #avatar-badge>
+              <slot name="avatar-badge"></slot>
+            </template>
+          </component>
+        </gl-intersection-observer>
         <discussion-notes-replies-wrapper :is-diff-discussion="discussion.diff_discussion">
           <toggle-replies-widget
             v-if="hasReplies"

spec/frontend/diffs/components/diff_discussions_spec.js

 import { GlIcon } from '@gitlab/ui';
 import { mount, createLocalVue } from '@vue/test-utils';
 import DiffDiscussions from '~/diffs/components/diff_discussions.vue';
+import { discussionIntersectionObserverHandlerFactory } from '~/diffs/utils/discussions';
 import { createStore } from '~/mr_notes/stores';
 import DiscussionNotes from '~/notes/components/discussion_notes.vue';
 import NoteableDiscussion from '~/notes/components/noteable_discussion.vue';
@@ -19,6 +20,9 @@ describe('DiffDiscussions', () => {
     store = createStore();
     wrapper = mount(localVue.extend(DiffDiscussions), {
       store,
+      provide: {
+        discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+      },
       propsData: {
         discussions: getDiscussionsMockData(),
         ...props,

spec/frontend/diffs/utils/discussions_spec.js

+import { discussionIntersectionObserverHandlerFactory } from '~/diffs/utils/discussions';
+
+describe('Diff Discussions Utils', () => {
+  describe('discussionIntersectionObserverHandlerFactory', () => {
+    it('creates a handler function', () => {
+      expect(discussionIntersectionObserverHandlerFactory()).toBeInstanceOf(Function);
+    });
+
+    describe('intersection observer handler', () => {
+      const functions = {
+        setCurrentDiscussionId: jest.fn(),
+        getPreviousUnresolvedDiscussionId: jest.fn().mockImplementation((id) => {
+          return Number(id) - 1;
+        }),
+      };
+      const defaultProcessableWrapper = {
+        entry: {
+          time: 0,
+          isIntersecting: true,
+          rootBounds: {
+            bottom: 0,
+          },
+          boundingClientRect: {
+            top: 0,
+          },
+        },
+        currentDiscussion: {
+          id: 1,
+        },
+        isFirstUnresolved: false,
+        isDiffsPage: true,
+      };
+      let handler;
+      let getMock;
+      let setMock;
+
+      beforeEach(() => {
+        functions.setCurrentDiscussionId.mockClear();
+        functions.getPreviousUnresolvedDiscussionId.mockClear();
+
+        defaultProcessableWrapper.functions = functions;
+
+        setMock = functions.setCurrentDiscussionId.mock;
+        getMock = functions.getPreviousUnresolvedDiscussionId.mock;
+        handler = discussionIntersectionObserverHandlerFactory();
+      });
+
+      it('debounces multiple simultaneous requests into one queue', () => {
+        handler(defaultProcessableWrapper);
+        handler(defaultProcessableWrapper);
+        handler(defaultProcessableWrapper);
+        handler(defaultProcessableWrapper);
+
+        expect(setTimeout).toHaveBeenCalledTimes(4);
+        expect(clearTimeout).toHaveBeenCalledTimes(3);
+
+        // By only advancing to one timer, we ensure it's all being batched into one queue
+        jest.advanceTimersToNextTimer();
+
+        expect(functions.setCurrentDiscussionId).toHaveBeenCalledTimes(4);
+      });
+
+      it('properly processes, sorts and executes the correct actions for a set of observed intersections', () => {
+        handler(defaultProcessableWrapper);
+        handler({
+          // This observation is here to be filtered out because it's a scrollDown
+          ...defaultProcessableWrapper,
+          entry: {
+            ...defaultProcessableWrapper.entry,
+            isIntersecting: false,
+            boundingClientRect: { top: 10 },
+            rootBounds: { bottom: 100 },
+          },
+        });
+        handler({
+          ...defaultProcessableWrapper,
+          entry: {
+            ...defaultProcessableWrapper.entry,
+            time: 101,
+            isIntersecting: false,
+            rootBounds: { bottom: -100 },
+          },
+          currentDiscussion: { id: 20 },
+        });
+        handler({
+          ...defaultProcessableWrapper,
+          entry: {
+            ...defaultProcessableWrapper.entry,
+            time: 100,
+            isIntersecting: false,
+            boundingClientRect: { top: 100 },
+          },
+          currentDiscussion: { id: 30 },
+          isDiffsPage: false,
+        });
+        handler({
+          ...defaultProcessableWrapper,
+          isFirstUnresolved: true,
+          entry: {
+            ...defaultProcessableWrapper.entry,
+            time: 100,
+            isIntersecting: false,
+            boundingClientRect: { top: 200 },
+          },
+        });
+
+        jest.advanceTimersToNextTimer();
+
+        expect(setMock.calls.length).toBe(4);
+        expect(setMock.calls[0]).toEqual([1]);
+        expect(setMock.calls[1]).toEqual([29]);
+        expect(setMock.calls[2]).toEqual([null]);
+        expect(setMock.calls[3]).toEqual([19]);
+
+        expect(getMock.calls.length).toBe(2);
+        expect(getMock.calls[0]).toEqual([30, false]);
+        expect(getMock.calls[1]).toEqual([20, true]);
+
+        [
+          setMock.invocationCallOrder[0],
+          getMock.invocationCallOrder[0],
+          setMock.invocationCallOrder[1],
+          setMock.invocationCallOrder[2],
+          getMock.invocationCallOrder[1],
+          setMock.invocationCallOrder[3],
+        ].forEach((order, idx, list) => {
+          // Compare each invocation sequence to the one before it (except the first one)
+          expect(list[idx - 1] || -1).toBeLessThan(order);
+        });
+      });
+    });
+  });
+});

spec/frontend/notes/components/discussion_notes_spec.js

 import { getByRole } from '@testing-library/dom';
 import { shallowMount, mount } from '@vue/test-utils';
 import '~/behaviors/markdown/render_gfm';
+import { discussionIntersectionObserverHandlerFactory } from '~/diffs/utils/discussions';
 import DiscussionNotes from '~/notes/components/discussion_notes.vue';
 import NoteableNote from '~/notes/components/noteable_note.vue';
 import { SYSTEM_NOTE } from '~/notes/constants';
@@ -26,6 +27,9 @@ describe('DiscussionNotes', () => {
   const createComponent = (props, mountingMethod = shallowMount) => {
     wrapper = mountingMethod(DiscussionNotes, {
       store,
+      provide: {
+        discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+      },
       propsData: {
         discussion: discussionMock,
         isExpanded: false,

spec/frontend/notes/components/noteable_discussion_spec.js

@@ -3,6 +3,7 @@ import { nextTick } from 'vue';
 import discussionWithTwoUnresolvedNotes from 'test_fixtures/merge_requests/resolved_diff_discussion.json';
 import { trimText } from 'helpers/text_helper';
 import mockDiffFile from 'jest/diffs/mock_data/diff_file';
+import { discussionIntersectionObserverHandlerFactory } from '~/diffs/utils/discussions';
 import DiscussionNotes from '~/notes/components/discussion_notes.vue';
 import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
 import ResolveWithIssueButton from '~/notes/components/discussion_resolve_with_issue_button.vue';
@@ -31,6 +32,9 @@ describe('noteable_discussion component', () => {
 
     wrapper = mount(NoteableDiscussion, {
       store,
+      provide: {
+        discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+      },
       propsData: { discussion: discussionMock },
     });
   });
@@ -167,6 +171,9 @@ describe('noteable_discussion component', () => {
 
         wrapper = mount(NoteableDiscussion, {
           store,
+          provide: {
+            discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+          },
           propsData: { discussion: discussionMock },
         });
       });
@@ -185,6 +192,9 @@ describe('noteable_discussion component', () => {
 
         wrapper = mount(NoteableDiscussion, {
           store,
+          provide: {
+            discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+          },
           propsData: { discussion: discussionMock },
         });
       });

spec/frontend/notes/components/notes_app_spec.js

@@ -9,6 +9,7 @@ import DraftNote from '~/batch_comments/components/draft_note.vue';
 import batchComments from '~/batch_comments/stores/modules/batch_comments';
 import axios from '~/lib/utils/axios_utils';
 import * as urlUtility from '~/lib/utils/url_utility';
+import { discussionIntersectionObserverHandlerFactory } from '~/diffs/utils/discussions';
 import CommentForm from '~/notes/components/comment_form.vue';
 import NotesApp from '~/notes/components/notes_app.vue';
 import * as constants from '~/notes/constants';
@@ -78,6 +79,9 @@ describe('note_app', () => {
           </div>`,
         },
         {
+          provide: {
+            discussionObserverHandler: discussionIntersectionObserverHandlerFactory(),
+          },
           propsData,
           store,
         },