Skip to content

Render the Super Sidebar earlier

Mark Florian requested to merge 391738-super-sidebar-perf into master

What does this MR do and why?

This makes the Super Sidebar render earlier in the page lifecycle, to improve perceived performance.

Before this change, the Super Sidebar often (most or all of the time) mounted after the page content/app(s). This is for two main reasons:

  • The sidebar was pulled out into an async chunk via a dynamic import().
  • This import was done inside a requestIdleCallback.

Compare this to regular page apps, which execute as soon as the initial document is parsed, thanks to the defer attributes set on our scripts.

With this change, the Super Sidebar is set up as early as controller/action entrypoints. Thanks to the placement of the entrypoint in <head>, it should set up before them, in fact.

Before After 
flowchart TD
    A(Document parsed) --> B(main chunk executes)
    B -.-> C(requestIdleCallback)
    C -.-> |"async import()"| D(initSuperSidebar)
    B --> E(Page controller/action entrypoints execute)
 
flowchart TD
    A(Document parsed) --> B(main chunk executes) 
    B --> E(Page controller/action entrypoints execute)
    B --> D(initSuperSidebar)

Screenshots or screen recordings

To replicate the SiteSpeed measurements below:

  • Apply this patch.
  • Download this login script.
  • Rename that script to give it a .js extension (GitLab doesn't allow downloading JS attachments).
  • Modify the URL in the script to point to your GDK instance.
  • Run something like docker run --rm -v "$(pwd):/sitespeed.io" --add-host gdk.test:172.16.123.1 sitespeedio/sitespeed.io:26.1.0 --preScript login.js 'https://gdk.test:3443/gitlab-org/gitlab-test/-/pipelines/charts', again, modifying the URL to point to your instance.
Before After Notes
1 1 The sidebar now appears at the same time as the page app.
Screenshot_2023-03-14_at_11-07-22_Summary_for_3_runs_https___gdk.test_3443_gitlab-org_gitlab-test_-_pipelines_charts_at_2023-03-13_13_09_24 Screenshot_2023-03-14_at_11-08-18_Summary_for_3_runs_https___gdk.test_3443_gitlab-org_gitlab-test_-_pipelines_charts_at_2023-03-13_13_21_11 The <SuperSidebar> component now gets created and mounted before the page app and tooltips/popovers apps.
Screenshot_2023-03-14_at_11-11-50_Summary_for_3_runs_https___gdk.test_3443_gitlab-org_gitlab-test_-_pipelines_charts_at_2023-03-13_13_09_24 Screenshot_2023-03-14_at_11-11-58_Summary_for_3_runs_https___gdk.test_3443_gitlab-org_gitlab-test_-_pipelines_charts_at_2023-03-13_13_21_11 We get to visual completeness/readiness earlier.

How to set up and validate locally

  1. Enable the super_sidebar_nav feature flag (e.g., Feature.enable(:super_sidebar_nav, User.first))
  2. Opt in to the new navigation via the user toggle (see !101910 (merged)).
  3. Visit a page with a relatively heavy page app, e.g., http://gdk.test:3000/gitlab-org/gitlab-test/-/pipelines/charts.
  4. Observe that the sidebar renders before the page app (or at least at the same time) rather than after.
  5. Observe that the top_nav chunk is not loaded.
  6. Opt out of the new navigation via the user toggle.
  7. Reload the page.
  8. Observe that the super_sidebar chunk is not loaded (using browser devtools or similar).

Other approaches

There were several other possible approaches I could have taken. Here they are, with a brief explanation of why I didn't take them:

  • Add webpack preload hint to existing dynamic import().
    • In my testing, the preload hint had no measureable effect.
  • Move dynamic import() out of requestIdleCallback
    • This improved the start up time for the sidebar, but it still often got set up after page apps.
  • Statically import initSuperSidebar in main.js, but call in requestIdleCallback
    • This increases the JS payload for all users, even if they don't have the new navigation enabled. This meant around 27kB (after gzip), which isn't huge, but it's also not nothing. Waiting for the idle callback only adds artificial delay.
  • Statically import initSuperSidebar in main.js, and call immediately.
    • Same as above, but a bit faster.

See #391738 (closed).

Reasons not to do this

  • Might delay DOMContentLoaded event for all pages. (Page scripts already do this.)
  • Eventually the old nav will be removed entirely, so we might as well just put the Super Sidebar in the main chunk right now.
  • The SuperSidebar now initialises too early - before popovers, tooltips, tracking... I doubt this matters, since page apps already do this.

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 #391738 (closed)

Edited by Mark Florian

Merge request reports