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

bugfix/cache-modal-performance #411

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BrianWhitneyAI
Copy link
Contributor

Description

The purpose of this PR is to resolve #409 and make the copy to local cache performant for 10,000+ files. This pr add a chunked response, waiting state and error handling around this process to make it smoother for users.

Testing

I tested this with the startup query date range that BFF has (with about 7,500 files) with no issues. Still investigating larger selections.

Copy link
Contributor

@SeanLeRoy SeanLeRoy left a comment

Choose a reason for hiding this comment

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

So did the caching fix fix this issue overall then?

packages/core/components/Modal/CopyFileManifest/index.tsx Outdated Show resolved Hide resolved
packages/core/components/Modal/CopyFileManifest/index.tsx Outdated Show resolved Hide resolved
const details = await fileSelection.fetchAllDetails();
setFileDetails(details);
} catch (err: any) {
throw new Error(err.message || "Error fetching file details.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This error won't get seen by a user in a useful manner. Having it be set to the component's state like you had earlier and then displaying the error would make this more useful

Copy link
Contributor

@pgarrison pgarrison left a comment

Choose a reason for hiding this comment

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

I think your recent changes made some of my comments unnecessary.

I tested this in prod and was able to successfully open the modal with over 6,000 files selected. The #412/#408 bug prevented me from selecting more files.

I also uncovered a UI bug that I suspect was already in BFF. I'll file a new issue

packages/core/components/Modal/CopyFileManifest/index.tsx Outdated Show resolved Hide resolved
<FileTable files={filesNotInLocalCache} title="Files to download to VAST" />
{loading && (
<div className={styles.loading}>
<p>Loading file details...</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thrilled to see this, I noticed the query time starts to get long even for hundreds of files, so this will really help users understand what's going on.

packages/core/components/Modal/CopyFileManifest/index.tsx Outdated Show resolved Hide resolved
packages/core/components/Modal/CopyFileManifest/index.tsx Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Copy to VAST with thousands of files crashes BFF
4 participants