Skip to content
Snippets Groups Projects

Load refs for the "Run Pipeline" form on demand, limit to 100 results per ref type

Merged Miguel Rincon requested to merge 321790-load-refs-on-demand into master
All threads resolved!

What does this MR do?

Improve performance of the "Run pipeline" page for projects with many branches or tags (refs).

Fix for: #322170 (closed) and #321790 (closed)

Background

In the original implementation of this page, we loaded all the tags and branches in the HTML payload.

Even though this was not ideal, this was cleverly done by adding the data to an inline javascript which loaded fast enough, although I has the potential to create a large amount of DOM elements in projects with a large amount of refs.

During the migration to Vue this problem was exacerbated as refs data was made reactive by Vue, and the number of DOM elements became higher, as the Vue dropdowns generate more DOM elements than the plain Javascript version.

Solution

Use the same JSON endpoint as the Repository page (e.g. in https://gitlab.com/gitlab-org/gitlab/-/tree/master) to load a limited number of refs and allow the user to filter by searching with a search box, as in the repository pages.

Screenshots (strongly suggested)

Using an example with 200 tags and 200 branches.

Before:

All refs are loaded to the page:

2021-03-02_09.45.13

After

Only 100 tags and 100 branches are loaded, and they are loaded only after the user opens the dropdown, additionally the search works by requesting the data to the backend instead of filtering in the frontend:

2021-03-02_09.47.47

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related to #321790 (closed)

Edited by Miguel Rincon

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 1 Warning
    :warning: This merge request is quite big (679 lines changed), please consider splitting it into multiple merge requests.
    1 Message
    :book: This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge.

    Documentation review

    The following files require a review from a technical writer:

    • doc/ci/pipelines/index.md

    The review does not need to block merging this merge request. See the:

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    Category Reviewer Maintainer
    ~changelog No reviewer available No maintainer available
    frontend Scott Stern (@sstern) (UTC-8, 9 hours behind @mrincon) Natalia Tepluhina (@ntepluhina) (UTC+1, same timezone as @mrincon)

    If needed, you can retry the danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 5cbdfdbd and e5826739

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 3.18 MB 3.18 MB - 0.0 %
    mainChunk 1.95 MB 1.95 MB - 0.0 %

    Please look at the full report for more details


    Read more about how this report works.

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • mentioned in issue #322170 (closed)

  • Miguel Rincon changed the description

    changed the description

  • Miguel Rincon
  • Miguel Rincon changed title from Load refs for the new pipeline form on demand to Load refs for the "Run Pipeline" form on demand, limit to 100 results

    changed title from Load refs for the new pipeline form on demand to Load refs for the "Run Pipeline" form on demand, limit to 100 results

  • Miguel Rincon changed title from Load refs for the "Run Pipeline" form on demand, limit to 100 results to Load refs for the "Run Pipeline" form on demand, limit to 100 results per ref type

    changed title from Load refs for the "Run Pipeline" form on demand, limit to 100 results to Load refs for the "Run Pipeline" form on demand, limit to 100 results per ref type

  • Miguel Rincon mentioned in merge request !54435 (closed)

    mentioned in merge request !54435 (closed)

  • Miguel Rincon added 1 commit

    added 1 commit

    • 8e7dd3aa - Load refs for the pipeline on demand

    Compare with previous version

  • Miguel Rincon added 1 commit

    added 1 commit

    • 4c15de23 - Load refs for the pipeline on demand

    Compare with previous version

  • Miguel Rincon marked the checklist item Changelog entry as completed

    marked the checklist item Changelog entry as completed

  • Miguel Rincon marked the checklist item Documentation (if required) as completed

    marked the checklist item Documentation (if required) as completed

  • Miguel Rincon marked the checklist item Code review guidelines as completed

    marked the checklist item Code review guidelines as completed

  • Miguel Rincon marked the checklist item Merge request performance guidelines as completed

    marked the checklist item Merge request performance guidelines as completed

  • Miguel Rincon marked the checklist item Style guides as completed

    marked the checklist item Style guides as completed

  • Miguel Rincon changed the description

    changed the description

  • Miguel Rincon added 1 commit

    added 1 commit

    • ddd63ea3 - Load refs for the pipeline on demand

    Compare with previous version

  • assigned to @mrincon

  • Miguel Rincon
  • requested review from @marcel.amirault

  • Miguel Rincon requested review from @sstern

    requested review from @sstern

  • Miguel Rincon requested review from @dskim_gitlab

    requested review from @dskim_gitlab

  • mentioned in issue #321790 (closed)

    • Author Maintainer
      Resolved by Miguel Rincon

      @v_mishra I think there are a few UX change to look forward to, could you take a look?

      • we get a loading icon when the user types as search
      • not all branches/tags are shown initially, as the data is loaded only after the user clicks on the dropdown
  • Miguel Rincon requested review from @v_mishra

    requested review from @v_mishra

  • I just noticed this but I think the border colors are different here.

    Screen_Shot_2021-03-02_at_1.57.18_PM

  • Payton Burdette
  • Sincheol (David) Kim requested review from @igor.drozdov and removed review request for @dskim_gitlab

    requested review from @igor.drozdov and removed review request for @dskim_gitlab

  • @mrincon thanks for your work on this MR. This endpoint is not currently flagged as needing changes for performance, ie: it is not on this board.

    If you are seeing otherwise please ping me or open an issue. I don't see that it needs a BE review at this time. (@igor.drozdov feel free to disagree :smile: )

  • Sean Carroll removed review request for @igor.drozdov

    removed review request for @igor.drozdov

  • Miguel Rincon added 1 commit

    added 1 commit

    • da5f2d12 - Load refs for the pipeline on demand

    Compare with previous version

  • Miguel Rincon mentioned in merge request !49575 (merged)

    mentioned in merge request !49575 (merged)

  • Miguel Rincon added 1 commit

    added 1 commit

    • e5826739 - Load refs for the pipeline on demand

    Compare with previous version

  • Scott Stern
  • Scott Stern approved this merge request

    approved this merge request

  • Miguel Rincon requested review from @ntepluhina

    requested review from @ntepluhina

  • Veethika Mishra removed review request for @v_mishra

    removed review request for @v_mishra

  • Miguel Rincon mentioned in issue #323444

    mentioned in issue #323444

  • Marcel Amirault approved this merge request

    approved this merge request

  • Natalia Tepluhina
  • Natalia Tepluhina
  • Natalia Tepluhina approved this merge request

    approved this merge request

  • Natalia Tepluhina resolved all threads

    resolved all threads

  • mentioned in commit c05acf6f

  • mentioned in issue #322386 (closed)

  • added workflowcanary label and removed workflowstaging label

  • Sean Carroll resolved all threads

    resolved all threads

  • added workflowproduction label and removed workflowcanary label

  • mentioned in issue #229632 (closed)

  • Samantha Ming mentioned in issue #343496

    mentioned in issue #343496

  • mentioned in issue #327085 (closed)

  • Scott Hampton mentioned in issue #358968

    mentioned in issue #358968

  • mentioned in issue #381233 (closed)

  • Please register or sign in to reply
    Loading