From 74417d7ef9580d175732a0bb6138facd386e34ea Mon Sep 17 00:00:00 2001 From: Brian Whitney Date: Wed, 5 Feb 2025 14:04:50 -0800 Subject: [PATCH 1/5] bugfix/cache-modal-performance --- .../Modal/CopyFileManifest/index.tsx | 113 ++++++++++++++---- 1 file changed, 90 insertions(+), 23 deletions(-) diff --git a/packages/core/components/Modal/CopyFileManifest/index.tsx b/packages/core/components/Modal/CopyFileManifest/index.tsx index 8deeecd2..c9686a03 100644 --- a/packages/core/components/Modal/CopyFileManifest/index.tsx +++ b/packages/core/components/Modal/CopyFileManifest/index.tsx @@ -1,5 +1,5 @@ -import filesize from "filesize"; import * as React from "react"; +import filesize from "filesize"; import { useDispatch, useSelector } from "react-redux"; import { ModalProps } from ".."; @@ -44,6 +44,11 @@ function FileTable({ files, title }: { files: FileDetail[]; title: string }) { return totalBytes ? filesize(totalBytes) : "Calculating..."; }; + // Check for bad files + if (files.length === 0 || files.some((file) => !file)) { + throw new Error("No file details available or invalid file details encountered."); + } + return (

{title}

@@ -60,8 +65,8 @@ function FileTable({ files, title }: { files: FileDetail[]; title: string }) { - {files.map((file) => ( - + {files.map((file, index) => ( + {clipFileName(file.name)} {filesize(file.size || 0)} @@ -72,9 +77,7 @@ function FileTable({ files, title }: { files: FileDetail[]; title: string }) { {hasScroll &&
}
- {files.length > 0 && ( - {calculateTotalSize(files)} - )} + {calculateTotalSize(files)} {files.length.toLocaleString()} files
@@ -91,14 +94,56 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { FileSelection.selectionsAreEqual ); + // State for file details, loading status, and errors. const [fileDetails, setFileDetails] = React.useState([]); + const [loading, setLoading] = React.useState(true); + const [error, setError] = React.useState(null); + // Progressive fetching of file details. React.useEffect(() => { - async function fetchDetails() { - const details = await fileSelection.fetchAllDetails(); - setFileDetails(details); + async function fetchDetailsIncrementally() { + try { + // Group file ranges by file set. + const fileRangesByFileSets = fileSelection.groupByFileSet(); + + // Array to hold promises that resolve to FileDetail[] + const promises: Promise[] = []; + + // For each file range, fetch details and update state as soon as they're available. + fileRangesByFileSets.forEach((ranges, fileSet) => { + ranges.forEach((range) => { + const promise: Promise = fileSet + .fetchFileRange(range.from, range.to) + .then((details) => { + details.forEach((file) => { + if (!file.id) { + throw new Error("File ID is not defined"); + } + }); + setFileDetails((prev) => [...prev, ...details]); + return details; + }) + .catch((err) => { + throw new Error( + `Error fetching files with range ${range.from}-${range.to}: ${err.message}` + ); + }); + promises.push(promise); + }); + }); + + // Wait for all the fetch promises to settle. + await Promise.allSettled(promises); + } catch (err: any) { + setError( + err.message || + "An error occurred while fetching file details. Please try again. If this issue persists try a smaller query." + ); + } finally { + setLoading(false); + } } - fetchDetails(); + fetchDetailsIncrementally(); }, [fileSelection]); const onMove = () => { @@ -124,18 +169,42 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { return ( -

- Files copied to the local storage (VAST) are stored with a 180-day - expiration, after which they revert to cloud-only storage. To extend the - expiration, reselect the files and confirm the update. -

- - + {loading && ( +
+

Loading file details...

+
+ )} + {error && ( +
+

Error

+

{error}

+
+ )} + {!loading && !error && ( + <> +

+ Files copied to the local storage (VAST) are stored with a 180-day + expiration, after which they revert to cloud-only storage. To extend + the expiration, reselect the files and confirm the update. +

+ {filesInLocalCache.length > 0 && ( + + )} + {filesNotInLocalCache.length > 0 && ( + + )} + + )} } footer={ @@ -148,15 +217,13 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { /> } - onDismiss={onDismiss} - title="Copy files to local storage (VAST)" /> ); } From a110b5eeed7059ebbbf71ea088852e02e25f4725 Mon Sep 17 00:00:00 2001 From: Brian Whitney Date: Wed, 5 Feb 2025 16:10:23 -0800 Subject: [PATCH 2/5] remove handling and add improved file selection --- .../Modal/CopyFileManifest/index.tsx | 63 +++---------------- packages/core/entity/FileSelection/index.ts | 18 +----- packages/core/entity/FileSet/index.ts | 27 +++++--- 3 files changed, 29 insertions(+), 79 deletions(-) diff --git a/packages/core/components/Modal/CopyFileManifest/index.tsx b/packages/core/components/Modal/CopyFileManifest/index.tsx index c9686a03..bda5b114 100644 --- a/packages/core/components/Modal/CopyFileManifest/index.tsx +++ b/packages/core/components/Modal/CopyFileManifest/index.tsx @@ -1,5 +1,5 @@ -import * as React from "react"; import filesize from "filesize"; +import * as React from "react"; import { useDispatch, useSelector } from "react-redux"; import { ModalProps } from ".."; @@ -39,16 +39,10 @@ function FileTable({ files, title }: { files: FileDetail[]; title: string }) { }; const calculateTotalSize = (files: FileDetail[]) => { - if (files.length === 0) return ""; const totalBytes = files.reduce((acc, file) => acc + (file.size || 0), 0); return totalBytes ? filesize(totalBytes) : "Calculating..."; }; - // Check for bad files - if (files.length === 0 || files.some((file) => !file)) { - throw new Error("No file details available or invalid file details encountered."); - } - return (

{title}

@@ -94,56 +88,21 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { FileSelection.selectionsAreEqual ); - // State for file details, loading status, and errors. const [fileDetails, setFileDetails] = React.useState([]); const [loading, setLoading] = React.useState(true); - const [error, setError] = React.useState(null); - // Progressive fetching of file details. React.useEffect(() => { - async function fetchDetailsIncrementally() { + async function fetchFiles() { try { - // Group file ranges by file set. - const fileRangesByFileSets = fileSelection.groupByFileSet(); - - // Array to hold promises that resolve to FileDetail[] - const promises: Promise[] = []; - - // For each file range, fetch details and update state as soon as they're available. - fileRangesByFileSets.forEach((ranges, fileSet) => { - ranges.forEach((range) => { - const promise: Promise = fileSet - .fetchFileRange(range.from, range.to) - .then((details) => { - details.forEach((file) => { - if (!file.id) { - throw new Error("File ID is not defined"); - } - }); - setFileDetails((prev) => [...prev, ...details]); - return details; - }) - .catch((err) => { - throw new Error( - `Error fetching files with range ${range.from}-${range.to}: ${err.message}` - ); - }); - promises.push(promise); - }); - }); - - // Wait for all the fetch promises to settle. - await Promise.allSettled(promises); + const details = await fileSelection.fetchAllDetails(); + setFileDetails(details); } catch (err: any) { - setError( - err.message || - "An error occurred while fetching file details. Please try again. If this issue persists try a smaller query." - ); + throw new Error(err.message || "Error fetching file details."); } finally { setLoading(false); } } - fetchDetailsIncrementally(); + fetchFiles(); }, [fileSelection]); const onMove = () => { @@ -178,13 +137,7 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) {

Loading file details...

)} - {error && ( -
-

Error

-

{error}

-
- )} - {!loading && !error && ( + {!loading && ( <>

Files copied to the local storage (VAST) are stored with a 180-day @@ -217,7 +170,7 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { /> { - const fileRangesByFileSets = this.groupByFileSet(); - // Load file metadata for every file selected (however do to some performance enhancements - // the fetch will overshoot) const fileRangePromises: Promise[] = []; - fileRangesByFileSets.forEach((ranges, fileSet) => { + this.groupByFileSet().forEach((ranges, fileSet) => { ranges.forEach((range) => { fileRangePromises.push(fileSet.fetchFileRange(range.from, range.to)); }); }); - await Promise.all(fileRangePromises); - - // Collect the desired files from the fetched files - const files: FileDetail[] = []; - fileRangesByFileSets.forEach((ranges, fileSet) => { - ranges.forEach((range) => { - for (let i = range.from; i <= range.to; i++) { - files.push(fileSet.getFileByIndex(i) as FileDetail); - } - }); - }); - return files; + return (await Promise.all(fileRangePromises)).flat(); } /** diff --git a/packages/core/entity/FileSet/index.ts b/packages/core/entity/FileSet/index.ts index ae744ecf..f99e2978 100644 --- a/packages/core/entity/FileSet/index.ts +++ b/packages/core/entity/FileSet/index.ts @@ -121,14 +121,25 @@ export default class FileSet { fileSet: this, }); - // Update cache for files fetched, due to overfetching the indexes updated - // in the cache will be inclusive of the range requested but may not necessarily start - // at the requested index, therefore here startIndexOfPage is used instead of startIndex - for (let i = 0; i < response.length; i++) { - this.cache.set(startIndexOfPage + i, response[i]); - } - - return response; + // Grab responses and shove them into the cache + // while also filtering out the files that outside the range + // requested for the response to the caller of fetchFileRange() + return response.reduce((filesInRange, file, idxInResponses) => { + // Update cache for files fetched, due to overfetching the indexes updated + // in the cache will be inclusive of the range requested but may not necessarily start + // at the requested index, therefore here startIndexOfPage is used instead of startIndex + const idxInFileSet = startIndexOfPage + idxInResponses; + this.cache.set(idxInFileSet, file); + + // If the file is within the range requested, add it to the files array + // for returning back to the caller of fetchFileRange(). + // Ranges are inclusive of their bounds + if (idxInFileSet >= startIndex && idxInFileSet <= endIndex) { + filesInRange.push(file); + } + + return filesInRange; + }, [] as FileDetail[]); } finally { // Clear the previously saved indexes as they are no longer loading for (let i = startIndexOfPage; i < startIndexOfPage + limit; i++) { From 009c40e0c239cbfa20c5e9713ff70c3e68b1cce3 Mon Sep 17 00:00:00 2001 From: Brian Whitney Date: Wed, 5 Feb 2025 16:19:54 -0800 Subject: [PATCH 3/5] remove unnecessary conditions --- packages/core/components/Modal/CopyFileManifest/index.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/components/Modal/CopyFileManifest/index.tsx b/packages/core/components/Modal/CopyFileManifest/index.tsx index bda5b114..feedaef4 100644 --- a/packages/core/components/Modal/CopyFileManifest/index.tsx +++ b/packages/core/components/Modal/CopyFileManifest/index.tsx @@ -59,8 +59,8 @@ function FileTable({ files, title }: { files: FileDetail[]; title: string }) { - {files.map((file, index) => ( - + {files.map((file) => ( + {clipFileName(file.name)} {filesize(file.size || 0)} @@ -170,7 +170,7 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { /> Date: Thu, 6 Feb 2025 11:44:17 -0800 Subject: [PATCH 4/5] add error state and update test --- .../components/Modal/CopyFileManifest/index.tsx | 14 +++++++++++--- packages/core/entity/FileSet/test/FileSet.test.ts | 2 +- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/core/components/Modal/CopyFileManifest/index.tsx b/packages/core/components/Modal/CopyFileManifest/index.tsx index feedaef4..773da21e 100644 --- a/packages/core/components/Modal/CopyFileManifest/index.tsx +++ b/packages/core/components/Modal/CopyFileManifest/index.tsx @@ -90,6 +90,7 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { const [fileDetails, setFileDetails] = React.useState([]); const [loading, setLoading] = React.useState(true); + const [error, setError] = React.useState(null); React.useEffect(() => { async function fetchFiles() { @@ -97,7 +98,8 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { const details = await fileSelection.fetchAllDetails(); setFileDetails(details); } catch (err: any) { - throw new Error(err.message || "Error fetching file details."); + console.error("Error fetching file details:", err); + setError(err.message || "Error fetching file details."); } finally { setLoading(false); } @@ -137,7 +139,13 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) {

Loading file details...

)} - {!loading && ( + {error && ( +
+

Error

+

{error}

+
+ )} + {!loading && !error && ( <>

Files copied to the local storage (VAST) are stored with a 180-day @@ -170,7 +178,7 @@ export default function CopyFileManifest({ onDismiss }: ModalProps) { /> { it("returns slices of the file list represented by the FileSet, specified by index position", async () => { const fileService = new HttpFileService(); - sandbox.replace(fileService, "getFiles", () => Promise.resolve(files.slice(1, 4))); + sandbox.replace(fileService, "getFiles", () => Promise.resolve(files.slice(0, 4))); const fileSet = new FileSet({ fileService }); expect(await fileSet.fetchFileRange(1, 3)).to.deep.equal(files.slice(1, 4)); // Array.prototype.slice is exclusive of end bound }); From 9a50a17ccac52c76450c3506f39314b34ea74309 Mon Sep 17 00:00:00 2001 From: Brian Whitney Date: Thu, 6 Feb 2025 11:45:48 -0800 Subject: [PATCH 5/5] update test --- packages/core/entity/FileSet/test/FileSet.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/entity/FileSet/test/FileSet.test.ts b/packages/core/entity/FileSet/test/FileSet.test.ts index aa176b94..3cc2130f 100644 --- a/packages/core/entity/FileSet/test/FileSet.test.ts +++ b/packages/core/entity/FileSet/test/FileSet.test.ts @@ -145,7 +145,7 @@ describe("FileSet", () => { const fileService = new HttpFileService(); sandbox.replace(fileService, "getFiles", () => Promise.resolve(files.slice(0, 4))); const fileSet = new FileSet({ fileService }); - expect(await fileSet.fetchFileRange(1, 3)).to.deep.equal(files.slice(1, 4)); // Array.prototype.slice is exclusive of end bound + expect(await fileSet.fetchFileRange(1, 3)).to.deep.equal(files.slice(1, 4)); }); it("turns indicies for requested data into a properly formed pagination query", async () => {