Merge pull request galaxyproject#17720 from ElectronicBlueberry/fix-c…

…ollections-fetch

[24.0] Fix Collection Scrolling not showing all items

client/src/components/History/CurrentCollection/CollectionPanel.vue

@@ -43,7 +43,16 @@ const dsc = computed(() => {
     }
     return currentCollection;
 });
-const collectionElements = computed(() => collectionElementsStore.getCollectionElements(dsc.value, offset.value));
+
+watch(
+    () => [dsc.value, offset.value],
+    () => {
+        collectionElementsStore.fetchMissingElements(dsc.value, offset.value);
+    },
+    { immediate: true }
+);
+
+const collectionElements = computed(() => collectionElementsStore.getCollectionElements(dsc.value) ?? []);
 const loading = computed(() => collectionElementsStore.isLoadingCollectionElements(dsc.value));
 const jobState = computed(() => ("job_state_summary" in dsc.value ? dsc.value.job_state_summary : undefined));
 const populatedStateMsg = computed(() =>
@@ -99,7 +108,8 @@ watch(
 watch(
     jobState,
     () => {
-        collectionElementsStore.loadCollectionElements(dsc.value);
+        collectionElementsStore.invalidateCollectionElements(dsc.value);
+        collectionElementsStore.fetchMissingElements(dsc.value, offset.value);
     },
     { deep: true }
 );

client/src/store/historyStore/model/utilities.js

@@ -1,9 +1,9 @@
-import Vue from "vue";
+import { set } from "vue";
 
 /* This function merges the existing data with new incoming data. */
 export function mergeArray(id, payload, items, itemKey) {
     if (!items[id]) {
-        Vue.set(items, id, []);
+        set(items, id, []);
     }
     const itemArray = items[id];
     for (const item of payload) {
@@ -16,7 +16,7 @@ export function mergeArray(id, payload, items, itemKey) {
                 });
             }
         } else {
-            Vue.set(itemArray, itemIndex, item);
+            set(itemArray, itemIndex, item);
         }
     }
 }

client/src/stores/collectionElementsStore.test.ts

@@ -38,13 +38,15 @@ describe("useCollectionElementsStore", () => {
         expect(store.storedCollectionElements).toEqual({});
         expect(store.isLoadingCollectionElements(collection)).toEqual(false);
 
-        const offset = 0;
+        // Getting collection elements should be side effect free
+        store.getCollectionElements(collection);
+        expect(store.isLoadingCollectionElements(collection)).toEqual(false);
+        await flushPromises();
+        expect(fetchCollectionElements).not.toHaveBeenCalled();
+
         const limit = 5;
-        // Getting collection elements should trigger a fetch and change the loading state
-        store.getCollectionElements(collection, offset, limit);
-        expect(store.isLoadingCollectionElements(collection)).toEqual(true);
+        store.fetchMissingElements(collection, 0, limit);
         await flushPromises();
-        expect(store.isLoadingCollectionElements(collection)).toEqual(false);
         expect(fetchCollectionElements).toHaveBeenCalled();
 
         const collectionKey = store.getCollectionKey(collection);
@@ -70,18 +72,20 @@ describe("useCollectionElementsStore", () => {
         const offset = 0;
         const limit = storedCount;
         // Getting the same collection elements range should not trigger a fetch
-        store.getCollectionElements(collection, offset, limit);
+        store.fetchMissingElements(collection, offset, limit);
         expect(store.isLoadingCollectionElements(collection)).toEqual(false);
         expect(fetchCollectionElements).not.toHaveBeenCalled();
     });
 
     it("should fetch only missing elements if the requested range is not already stored", async () => {
+        jest.useFakeTimers();
+
         const totalElements = 10;
         const collection: HDCASummary = mockCollection("1", totalElements);
         const store = useCollectionElementsStore();
 
         const initialElements = 3;
-        store.getCollectionElements(collection, 0, initialElements);
+        store.fetchMissingElements(collection, 0, initialElements);
         await flushPromises();
         expect(fetchCollectionElements).toHaveBeenCalled();
         const collectionKey = store.getCollectionKey(collection);
@@ -92,11 +96,10 @@ describe("useCollectionElementsStore", () => {
 
         const offset = 2;
         const limit = 5;
-        // Getting collection elements should trigger a fetch in this case
-        store.getCollectionElements(collection, offset, limit);
-        expect(store.isLoadingCollectionElements(collection)).toEqual(true);
+        // Fetching collection elements should trigger a fetch in this case
+        store.fetchMissingElements(collection, offset, limit);
+        jest.runAllTimers();
         await flushPromises();
-        expect(store.isLoadingCollectionElements(collection)).toEqual(false);
         expect(fetchCollectionElements).toHaveBeenCalled();
 
         elements = store.storedCollectionElements[collectionKey];

client/src/stores/collectionElementsStore.ts

@@ -4,6 +4,8 @@ import { computed, del, ref, set } from "vue";
 import type { CollectionEntry, DCESummary, HDCASummary, HistoryContentItemBase } from "@/api";
 import { isHDCA } from "@/api";
 import { fetchCollectionDetails, fetchElementsFromCollection } from "@/api/datasetCollections";
+import { ensureDefined } from "@/utils/assertions";
+import { ActionSkippedError, LastQueue } from "@/utils/lastQueue";
 
 /**
  * Represents an element in a collection that has not been fetched yet.
@@ -24,7 +26,11 @@ export interface ContentPlaceholder {
     fetching?: boolean;
 }
 
-export type DCEEntry = ContentPlaceholder | DCESummary;
+export type InvalidDCEEntry = (ContentPlaceholder | DCESummary) & {
+    valid: false;
+};
+
+export type DCEEntry = ContentPlaceholder | DCESummary | InvalidDCEEntry;
 
 const FETCH_LIMIT = 50;
 
@@ -46,11 +52,8 @@ export const useCollectionElementsStore = defineStore("collectionElementsStore",
     }
 
     const getCollectionElements = computed(() => {
-        return (collection: CollectionEntry, offset = 0, limit = FETCH_LIMIT) => {
-            const storedElements =
-                storedCollectionElements.value[getCollectionKey(collection)] ?? initWithPlaceholderElements(collection);
-            fetchMissingElements({ collection, storedElements, offset, limit });
-            return storedElements;
+        return (collection: CollectionEntry) => {
+            return storedCollectionElements.value[getCollectionKey(collection)];
         };
     });
 
@@ -60,47 +63,91 @@ export const useCollectionElementsStore = defineStore("collectionElementsStore",
         };
     });
 
-    async function fetchMissingElements(params: {
-        collection: CollectionEntry;
+    type FetchParams = {
         storedElements: DCEEntry[];
+        collection: CollectionEntry;
         offset: number;
         limit: number;
-    }) {
-        const collectionKey = getCollectionKey(params.collection);
+    };
+
+    async function fetchMissing({ storedElements, collection, offset, limit = FETCH_LIMIT }: FetchParams) {
+        const collectionKey = getCollectionKey(collection);
+
         try {
-            // We should fetch only missing (placeholder) elements from the range
-            const firstMissingIndexInRange = params.storedElements
-                .slice(params.offset, params.offset + params.limit)
-                .findIndex((element) => isPlaceholder(element) && !element.fetching);
-
-            if (firstMissingIndexInRange === -1) {
-                // All elements in the range are already stored or being fetched
-                return;
+            if (collection.element_count !== null) {
+                // We should fetch only missing (placeholder) elements from the range
+                const firstMissingIndexInRange = storedElements
+                    .slice(offset, offset + limit)
+                    .findIndex((element) => (isPlaceholder(element) && !element.fetching) || isInvalid(element));
+
+                if (firstMissingIndexInRange === -1) {
+                    // All elements in the range are already stored or being fetched
+                    return;
+                }
+
+                // Adjust the offset to the first missing element
+                offset += firstMissingIndexInRange;
+            } else {
+                // Edge case where element_count is incorrect
+                // TODO: remove me once element_count is reported reliably
+                offset = 0;
             }
-            // Adjust the offset to the first missing element
-            params.offset += firstMissingIndexInRange;
 
             set(loadingCollectionElements.value, collectionKey, true);
             // Mark all elements in the range as fetching
-            params.storedElements
-                .slice(params.offset, params.offset + params.limit)
+            storedElements
+                .slice(offset, offset + limit)
                 .forEach((element) => isPlaceholder(element) && (element.fetching = true));
+
             const fetchedElements = await fetchElementsFromCollection({
-                entry: params.collection,
-                offset: params.offset,
-                limit: params.limit,
+                entry: collection,
+                offset: offset,
+                limit: limit,
             });
-            // Update only the elements that were fetched
-            params.storedElements.splice(params.offset, fetchedElements.length, ...fetchedElements);
-            set(storedCollectionElements.value, collectionKey, params.storedElements);
+
+            return { fetchedElements, elementOffset: offset };
         } finally {
             del(loadingCollectionElements.value, collectionKey);
         }
     }
 
-    async function loadCollectionElements(collection: CollectionEntry) {
-        const elements = await fetchElementsFromCollection({ entry: collection });
-        set(storedCollectionElements.value, getCollectionKey(collection), elements);
+    const lastQueue = new LastQueue<typeof fetchMissing>(1000, true);
+
+    async function fetchMissingElements(collection: CollectionEntry, offset: number, limit = FETCH_LIMIT) {
+        const key = getCollectionKey(collection);
+        let storedElements = storedCollectionElements.value[key];
+
+        if (!storedElements) {
+            storedElements = initWithPlaceholderElements(collection);
+            set(storedCollectionElements.value, key, storedElements);
+        }
+
+        try {
+            const data = await lastQueue.enqueue(fetchMissing, { storedElements, collection, offset, limit }, key);
+
+            if (data) {
+                const from = data.elementOffset;
+                const to = from + data.fetchedElements.length;
+
+                for (let index = from; index < to; index++) {
+                    const element = ensureDefined(data.fetchedElements[index - from]);
+                    set(storedElements, index, element);
+                }
+
+                set(storedCollectionElements.value, key, storedElements);
+            }
+        } catch (e) {
+            if (!(e instanceof ActionSkippedError)) {
+                throw e;
+            }
+        }
+    }
+
+    async function invalidateCollectionElements(collection: CollectionEntry) {
+        const storedElements = storedCollectionElements.value[getCollectionKey(collection)] ?? [];
+        storedElements.forEach((element) => {
+            (element as InvalidDCEEntry).valid = false;
+        });
     }
 
     function saveCollections(historyContentsPayload: HistoryContentItemBase[]) {
@@ -135,6 +182,10 @@ export const useCollectionElementsStore = defineStore("collectionElementsStore",
         return "id" in element === false;
     }
 
+    function isInvalid(element: DCEEntry): element is InvalidDCEEntry {
+        return (element as InvalidDCEEntry)["valid"] === false;
+    }
+
     function initWithPlaceholderElements(collection: CollectionEntry): ContentPlaceholder[] {
         const totalElements = collection.element_count ?? 0;
         const placeholderElements = new Array<ContentPlaceholder>(totalElements);
@@ -151,8 +202,9 @@ export const useCollectionElementsStore = defineStore("collectionElementsStore",
         isLoadingCollectionElements,
         getCollection,
         fetchCollection,
-        loadCollectionElements,
+        invalidateCollectionElements,
         saveCollections,
         getCollectionKey,
+        fetchMissingElements,
     };
 });

client/src/utils/lastQueue.ts

@@ -1,4 +1,4 @@
-type QueuedAction<T extends (...args: any) => R, R = unknown> = {
+type QueuedAction<T extends (...args: any) => R, R = ReturnType<T>> = {
     action: T;
     arg: Parameters<T>[0];
     resolve: (value: R) => void;
@@ -13,7 +13,7 @@ export class ActionSkippedError extends Error {}
  * This is useful when promises earlier enqueued become obsolete.
  * See also: https://stackoverflow.com/questions/53540348/js-async-await-tasks-queue
  */
-export class LastQueue<T extends (arg: any) => R, R = unknown> {
+export class LastQueue<T extends (arg: any) => R, R = ReturnType<T>> {
     throttlePeriod: number;
     /** Throw an error if a queued action is skipped. This avoids dangling promises */
     rejectSkipped: boolean;
@@ -34,7 +34,7 @@ export class LastQueue<T extends (arg: any) => R, R = unknown> {
         promise?.reject(new ActionSkippedError());
     }
 
-    async enqueue(action: T, arg: Parameters<T>[0], key: string | number = 0) {
+    async enqueue(action: T, arg: Parameters<T>[0], key: string | number = 0): Promise<R> {
         return new Promise((resolve, reject) => {
             this.skipPromise(key);
             this.queuedPromises[key] = { action, arg, resolve, reject };