Keep following linked note while notes load

Oleg Yakovenkorequested to merge
630496-keep-following-note-permalink into master

What does this MR do and why?

This fixes two bugs in scrollToTargetOnResize in app/assets/javascripts/lib/utils/resize_observer.js. The util is used by work_item_notes.vue (issues, epics and other work items) and by notes_app.vue (merge requests), so the change affects both.

Opening a comment permalink loads notes in pages of 15. The util watches the notes container with a ResizeObserver and scrolls the linked note to the top each time a new page of notes loads. It stops once an IntersectionObserver reports the note has left the viewport.

Bug 1: Vue replaces the note node as more notes load. A detached node reports isIntersecting: false, so the first replacement made the util stop, before the rest of the notes had loaded.

Fix 1: the intersection callback now checks that the node is still connected to the document before it stops. On each resize, if the note has a new node, the util moves its observer to that node. Until then, the scroll handler skips events, because a detached node measures as all zeros. cleanup() unobserves the node it observed, so it no longer passes null when the note is briefly missing.

Bug 2: the util's own scrollTo call fires a scroll event. While the page is still loading, the browser clamps the requested position, and the scroll handler stored the clamped gap as a manual scroll. Every later correction then landed short by that amount.

Fix 2: after scrollTo, the util records where the page landed. The scroll handler ignores scroll events at that exact position.

gitlab.com timeline

Recorded on gitlab-org#6044 (comment 3788021818) (an epic with 130 notes), current production code:

Time Event
3.68s Note appears, util scrolls to it and starts observing
4.00s Vue replaces the note node
5.01s Util stops (1s after the replacement, bug 1)
7.7s to 13.8s Node replaced 7 more times, no scrolling
final Scroll position 5231px, note is about 16900px further down

The first scroll requested 1454px while the max scroll was 1113px, which is the clamping case in bug 2.

References

Resolves #630496 (closed)

#416567 (closed): the page could randomly jump to the comment anchor. This is why cleanup() exists.

#526518 (closed): reacting to a deep-linked note re-rendered it and moved the page.

Screenshots or screen recordings

There is no static screenshot for this change, since it changes scroll position while notes load. The table below shows a before and after run using a simulated node swap, described in the validate section.

Before (master) After (this MR)
Util stops following 4.91s, 1.3s after the first swap 20.70s, after all 174 notes loaded
Scroll corrections 1 11
Watches the swapped-in node never after every swap
Final scroll position 1445px 31860px
Linked note position 30414px below the top of the scroll area at the top of the scroll area

On work items, the top of the scroll area sits behind the work item sticky header, so a one-line note there is covered. contentTop() does not count that header. This happens on master too and is tracked in #631223.

How to set up and validate locally

Work item (work_item_notes.vue)

  1. Seed an issue with 200 notes. In rails runner:

    issue = Project.find_by_full_path('flightjs/Flight').issues.first
root = User.find_by(username: 'root')
200.times do |i|
  Note.create!(
    noteable: issue,
    project: issue.project,
    author: root,
    note: "Note #{i}\n\nParagraph one of the note body.\n\nParagraph two, with more text to make the note taller."
  )
end

  2. Open a permalink to a note in the middle of the list, in a visible foreground browser tab. ResizeObserver does not run in a hidden tab.

  3. Wait for all notes to finish loading.

  4. Check that the note stays at the top of the page for the whole load. On work items, the first line of the note sits under the work item sticky header, because contentTop() does not count it. This happens on master too and is tracked in #631223.

On work items, the DOM node swap from bug 1 does not happen on its own in a local GDK, even with the same notes copied from a gitlab.com epic. To see bug 1 locally, simulate the swap with a small script in the browser console.

  1. Open a permalink to a note that is not on the first page of notes, in a visible foreground tab.
  2. Right after the page starts loading, paste the snippet below in the browser console.
  3. Wait for all notes to load.
  4. On master, the page stops following about a second after the first swap and the note ends far below the view. With this MR, the note stays at the top.
Swap snippet 
const ID = 'note_1989'; // change to the id from the URL hash
let lastCount = 0;
new MutationObserver(() => {
  const count = document.querySelectorAll('li[id^="note_"]').length;
  const orig = document.getElementById(ID);
  if (!orig || count < lastCount + 5 || orig.dataset.clone) return;
  lastCount = count;
  const clone = orig.cloneNode(true);
  clone.dataset.clone = '1';
  orig.replaceWith(clone);
  setTimeout(() => clone.isConnected && clone.replaceWith(orig), 300);
}).observe(document.body, { childList: true, subtree: true });

Merge request (notes_app.vue)

  1. Seed a merge request with 150 notes in rails runner:

    mr = MergeRequest.opened.first
root = User.find_by(username: 'root')
150.times do |i|
  Note.create!(
    noteable: mr,
    project: mr.project,
    author: root,
    note: "Note #{i}\n\nParagraph one of the note body.\n\nParagraph two, with more text to make the note taller."
  )
end

  2. Open a permalink to a note in the middle of the list on the MR Overview tab, in a visible foreground tab.

  3. Wait for all notes to load.

  4. Check that the note ends at the top, fully visible under the MR sticky header. On master it ends about 100px lower (bug 2).

The MR notes app also replaces the note node while notes load, so on master the util stops following about a second later (bug 1). MR notes load in large batches, so by then most notes have loaded and the page does not drift far.

The final check is on gitlab.com after deploy, with the epic link from the timeline above.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Oleg Yakovenko

Merge request reports