Updating nodes in tree with virtual scroll breaks tree - like load more nodes function

[tobiasengelhardt]

Minimal reproduction of the bug/regression with instructions:

Updating nodes and calling treeModel.update() with virtual scroll leads to a collapsing tree and finally a tree stuck in loading... This is only present in virtual scroll. Setting virtual scroll to false will fix the problem.

Stackblitz uses an example where the child nodes are loaded in steps with a load more functionality 100 nodes each time.

Link to Stackblitz reproduction

Repo steps:

• Open root node
• Click load more
• Root node will collapse
• If this is repeated multiple times tree is stuck in loading...

Expected behavior:

Tree behaves the same in virtual and non-virtual scroll.

Versions of Angular Tree Component, Angular, Node, affected browser(s) and operating system(s):

First noticed in version 8.5, still there in 10.0.0.

Other information:

[Cowraider]

Hi, what is the current status of this issue? We are experiencing performance issues with the tree that's why we want to enable virtual scroll. If no one is already working on it I would like to take a look into it, if that's ok for you?

[tobiasengelhardt]

We disabled virtual scroll in our application and have not seen any big performance issues. But showing more than 200 nodes is an edge case for us. With that and all that 2020 madness happening this issue has not been our focus. Feal free to take a look into this if you like! That would be awesome! I played around with it but did not get far. There is also the idea of moving from mobx to rxjs which would impact this issue.

[Cowraider]

I totally understand that it has not been your focus - having so many nodes is also an edge case for us but it still occurs sometimes and then it gets very slow.

I dug into it and I found that NOT adding the "scroll" event listener outside the angular zone "fixes" the issue. In the tree-viewport.component.ts file in the ngAfterViewInit just having el.addEventListener('scroll', this.scrollEventHandler); instead of this.ngZone.runOutsideAngular(() => { el.addEventListener('scroll', this.scrollEventHandler); }); helps.

I suspected that it had to do with the angular zone because when you click inside the container it renders it (in your reproduction). Unfortunately I cannot yet explain WHY this helps, but I will dig further into it. Also I am not sure why this code was added in the first place, as it was added in the following pull request #787 with the reason that it might negatively impact the performance (but probably not confirmed).

Anyway, if I find out why it helps and how I can fix it, I will create a PR.

[alechemy]

@Cowraider -- Not sure if you've made any progress on this, but I can try to assist if necessary. I authored the linked PR that moved the scroll event handler outside of ngZone.

In our application, we were seeing abysmal performance when using virtual scroll with large trees. A performance audit revealed that the scroll events were causing a huge number of change detection runs. Moving the event handler outside of the zone seemed to help somewhat, but later on (after my PR was merged) we discovered the same problem as you've described, where the tree can break when updated. So, I think my (attempted) fix may have caused more harm than good. 😞

[Cowraider]

Hi @alechemy, yes I have made progress, I got it to work by removing the this.ngZone.runOutsideAngular code for the scrolling. However, since I was not exactly sure why this helped (the statemanagement within the tree was not so easy to debug), which is why I wanted to wait until I was sure. For our project I forked the tree and reverted the this.ngZone.runOutsideAngular change and we didn't experience any problems since, and the virtual scroll feature makes the tree extremely fast - even with more than a few thousand nodes. Our plan is to move back to the original tree, once this is fixed.

If you want you could try to use my fork of the tree and see if it solves the performance problems as well as the loading problems for you (which I am confident it does). And I would create the PR so that we hopefully can merge this into this repository. The link for the forked npm package would be NPM package. The imports need to be adjusted

[tobiasengelhardt]

Can confirm that removing the ngZone fixes the update issue and has very good performance. @Cowraider, when you have your PR ready we will merge it into the repo.

Because this issue is in a few versions we will release this fix in all the mayor versions with the @circlon namespace (11, 10 and 9).

[Cowraider]

@tobiasengelhardt It seems that I don't have permissions to create a branch (and therefore am not able to create the PR). Is there anything I need to do, or am I missing something?

[tobiasengelhardt]

@Cowraider, you have to create the branch in the fork inside your account. With that branch you can open a PR against this repo. You probably need to update your fork to not have a conflict, because the tree-viewport had a recent change for the lodash removal.

[alechemy]

🎉 Great work @Cowraider , and my apologies for inadvertently creating this issue.