Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KED-1923] Add LazyList component using IntersectionObserver on sidebar #307

Merged
merged 24 commits into from
Dec 14, 2020

Conversation

bru5
Copy link
Contributor

@bru5 bru5 commented Nov 16, 2020

Description

Adds lazy list rendering and memoization to the sidebar.

Improves time to first paint and first interactive when loading a pipeline with a large number of elements, may also improve general sidebar interaction performance such as search and hover.

Development notes

Memoization

The NodeListRow component is now memoised using React.memo, checking minimal props that may change after first render, giving a performance boost on rendering each row that doesn't change (so most of them).

LazyList

Adds a new LazyList component that renders only the children that are currently visible on screen at any time.

Motivation

Some pipelines may have hundreds of elements and tags, which means a lot of sidebar elements to render, causing a slow time to interactive.

I found most components available in this space appear to be optimised for a specific kind of layout and interaction model, usually assuming a single flat list inside a single scrolling container. Even a specialised tree component such as react-vtree would likely limit potential future design directions.

In other cases significant reworking of our existing components and data structures would be needed, requiring special handling of many existing and future interactions in the sidebar.

An automatic LazyList implementation using IntersectionObserver

The LazyList component dynamically renders only the children currently visible at any time. Most user interactions should therefore work automatically, such as scroll, resize, but the key difference being that pretty much any kind of layout change, in any part of the app, at any time, will also be handled too with no extra code.

This is currently achieved using IntersectionObserver, with no other handlers being required (e.g. no scroll or resize, so far anyway).

It also does not require any virtual scrolling or component pooling to get full windowing from this either, as React's virtual DOM diffing and memoization appears to handle that fast enough by default, with a little buffering. But in practice, we have no need for this in our case so disposal has been switched off for smoother results.

The same approach can be extended to work with grids or any other kind of layout assuming the user could provide dimension / selection functions for it.

The actual implementation here is a little more involved than expected due to the details of how IntersectionObserver works and the React render cycle itself, but even with special care to avoid render thrashing, it still turns out to be really compact when using hooks (~200 lines total).

Caveats

  • requires IntersectionObserver

    • but is well supported by now
    • has an automatic fallback to full-list rendering (done)
  • scrolling fast briefly leaves a blank space

    • this is mostly mitigated by adding a buffer at both ends (done)
    • option to avoid disposal of rows helps avoid this too (done)
    • using a gradient fade on the ends of the list smooths this over visually (done)
    • can add a placeholder loading effect on the end too (later)
  • blocking the main thread leaves a blank space

    • we should aim to avoid doing this anyway
    • would likely be an issue for most other implementations too
    • currently hovering nodes causes the main thread to block
      • mitigated by disabling thread blocking hover effects (done for demo only)
      • a better solution is really needed here

Flag

This feature will be included behind a flag until it is well proven.

QA notes

All of the sidebar features should be tested thoroughly, in all supported browsers, in combination with switching pipelines.
Checking closely for any missing rows is important.

Some console messages are currently left in for demonstration and debugging.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@bru5 bru5 requested a review from richardwestenra November 16, 2020 18:36
@bru5 bru5 changed the title [KED-1923] Add LazyList component implementing lazy sidebar rendering [KED-1923] Add LazyList component using IntersectionObserver on sidebar Nov 16, 2020
Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, great. These notes are mostly for things that I think you're already aware of - I'm mainly adding comments just to remind us to check them so we don't forget.

src/components/node-list/styles/_group.scss Outdated Show resolved Hide resolved
src/components/node-list/node-list-group.js Outdated Show resolved Hide resolved
src/components/node-list/node-list-row.js Show resolved Hide resolved
src/components/node-list/node-list-group.js Outdated Show resolved Hide resolved
@bru5 bru5 marked this pull request as ready for review November 18, 2020 12:20
@bru5 bru5 requested a review from yetudada as a code owner November 18, 2020 15:15
Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR reduced the NodeList render time on my large test dataset from ~1690ms to ~30ms. Exceptional work!

Please don't forget to remove the console.logs before merging! :shipit:

@richardwestenra
Copy link
Contributor

oh also, you'll need to fix the test-lib tests unfortunately @bru5

@richardwestenra richardwestenra removed the request for review from yetudada November 27, 2020 15:38
README.md Outdated Show resolved Hide resolved
@studioswong
Copy link
Contributor

I've tested the lazyload sidebar on both chrome and safari with a large dataset, looks good! @bru5 looks like the test richard mentioned above still needs to be fixed?

Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work!

src/components/lazy-list/index.js Show resolved Hide resolved
src/utils/changed.js Outdated Show resolved Hide resolved
# Conflicts:
#	README.md
#	src/config.js
@bru5 bru5 merged commit 2edbddd into main Dec 14, 2020
@bru5 bru5 deleted the feature/lazy-sidebar branch December 14, 2020 16:51
@richardwestenra richardwestenra mentioned this pull request Dec 16, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants