Skip to content

Fix discussion_navigation scrolling unresolvable discussions

Justin Ho Tuan Duong requested to merge 381503-merge-request-notes-in-a-project-migrated-from-github-cannot-be-resolved into master

What does this MR do and why?

The previous discussion_navigation query selector only selected discussions that did not have the data-discussion-resolved attribute. This caused a bug where discussions which were not resolvable were still included when scrolling through unresolved discussions (even though the counter was correct).

This fix adds the data-discussion-resolvable attribute to such discussions and filters them out in the query selector.

The change also updates specs to be closer to actual user interaction by using mock scroll positions to determine which element should be scrolled into view.

Changelog: fixed

Screenshots or screen recordings

Note: I've recorded before / after videos that show how the new navigation is different. Focus on the first 2 discussions which are imported from GitHub. In the before video, you can still scroll through these 2 discussions even though they are not resolvable and are not included in the "unresolved threads" count. The after video fixes this behaviour.

Before After
Discussion_navigation_before Discussion_navigation_after

How to set up and validate locally

Note: This might not be the easiest to replicate but here are the main steps:

  1. (If you don't already have one) Create a GitHub project (yes GitHub) that can be tested on.
  2. Create a Pull Request with changes to at least 1 file (you can test with image / binary files as well). Add a file comment (a comment on the whole file, has a separate button on the top right) and some normal comments.
  3. Open GitLab, in the sidebar select + > New project/repository.
  4. Select Import project > GitHub. Then fill in a GitHub-issues Personal Access Token and click Authenticate.
  5. Select the project from 1 for import. Once the import is complete, view the Merge Request that was created from importing the PR from step 2.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #381503 (closed)

Merge request reports