Virtualize the Project Files tree list #2458
Conversation
There was a problem hiding this comment.
vue-virtual-scroller doesn't have any types in its package. This keeps us from getting yelled at from tsc
| @@ -1,37 +1,47 @@ | |||
| import { ContentRecordFile } from "../../../../src/api"; | |||
|
|
|||
| export function splitFilesOnInclusion( | |||
There was a problem hiding this comment.
This was no longer used after this work.
| @@ -1,14 +1,18 @@ | |||
| import { ContentRecordFile, FileMatchSource } from "../../../../../../src/api"; | |||
|
|
|||
| export function includedFileTooltip(file: ContentRecordFile) { | |||
| export function includedFileTooltip( | |||
| file: Pick<ContentRecordFile, "rel" | "reason">, | |||
There was a problem hiding this comment.
These changes allowed for easy usage for ContentRecordFile and the FlatFile type.
| // If the root file has changed, reset the expanded directories | ||
| if (msg.content.files.abs !== fileStore.files?.abs) { | ||
| fileStore.expandedDirs = new Set(); | ||
| } |
There was a problem hiding this comment.
The expanded directories only reset when the root changes; meaning switching deployments that have the same directory won't reset it.
This feels like good behavior to me, but I want to highlight it for discussion.
dotNomad
requested review from
kgartland-rstudio,
sagerb and
marcosnav
as code owners
| // If the root file has changed, reset the expanded directories | ||
| if (msg.content.files.abs !== fileStore.files?.abs) { | ||
| fileStore.expandedDirs = new Set(); | ||
| } |
| return { vscodeAPI }; | ||
| }); | ||
|
|
||
| describe("File Store", () => { |
There was a problem hiding this comment.
Thanks for adding these tests 🙇 !
There was a problem hiding this comment.
Validated and works great
Tried the previous implementation first, and the lag was really bad with a couple thousands of files.
Then, tried this new implementation and the files navigation and load times are much much better. 🎉
If the Files API takes awhile due to a very large directory size, and the user clicks a checkbox to include/exclude a item; the checkbox could be inaccurate when the parent directory is then collapsed and re-expanded.
Stumbled with the inaccurate checkbox state, it might deserve it's own GH issue. On the other hand, I was clicking and opening directories fiercely to see if the config file updated as expected, something that I don't think many users will do.
Agreed on all accounts. I'll create another issue for this. It is pretty separate logically from this, and to your point I don't think a lot of users will be opening and closing directories nearly as much as we are 😆 |
|
Created #2467 for the inaccurate state for the checkbox while waiting for the Files API. Thanks for prompting that @marcosnav - going to get this merged. |
Forgive the size on this one 😅. This PR improves the performance of the extension sidebar when Project Files has a large number of files. The list is now virtualized, only including the visible items in the DOM.
This is accomplished using
vue-virtual-scroller. I chose it since it is the top recommendation under Vue's documentation for Virtualize Large Lists optimizations.Intent
Resolves #2204
Type of Change
Approach
To keep things a bit more isolated I split out a new Files store from our Home store.
Only
TreeItemCheckboxneeded to change for this fix, but all functionality added was optional. This let me make the same functional changes toTreeItemso it could be used virtually in the future without affecting current usage.The Flat File array to make this all work is calculated based on the files we get from the API and the expanded directories set.
Virtualization
vue-virtual-scrollerhas an important notes section for itsRecycleScrollerthat is very relevant to the approach taken here:I'll walk through each that are important:
22pxin the same way VS Code handles sizingmax-heightwas used in CSSProjectFilecomponent was created to handle this, and the virtual scroller added to theProjectFilesview. TheTreeProjectFilescomponent was removed entirely..vue-recycle-scroller__item-view.hoveror.hover .some-element-inside-the-item-view)."TreeItemandTreeItemCheckboxAnother important note from this section: "The browsers have a size limitation on DOM elements, it means that currently the virtual scroller can't display more than ~500k items depending on the browser."
User Impact
A user sees very little impact unless they were running into the hanging sidebar described in #2204.
Expanded folders are now remembered until the deployment's directory changes. This mimics behavior seen in VS Code's Explorer view better where a parent collapsing doesn't reset the children's expanded state.
Automated Tests
Automated tests were added for the methods in the new File store.
Directions for Reviewers
Test with deployments with varying number of files. An ideal test would be a deployment with a directory containing 1000+ files.
Include, Exclude, Open actions should all work as they did before.
Note: that this PR doesn't change the speed of any of the files APIs. On a significantly large deployment directory the API could take upwards of 2 seconds. That bottleneck still exists, but is much less significant.
If the Files API takes awhile due to a very large directory size, and the user clicks a checkbox to include/exclude a item; the checkbox could be inaccurate when the parent directory is then collapsed and re-expanded. This is due to the item being removed then re-added to the DOM upon collapsing and expanding. We do not store a local state of included/excluded files and instead rely on the API response. This reliance also explains why including/excluding a directory does not change nested files until we hear back from the Files API.