From bc6a1a1d3b73582b651190011f82e8afd5d57e02 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 1 Aug 2026 21:49:01 -0400 Subject: [PATCH 1/2] fix(ui): stop range-based fetching hooks from spinning in a render loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fetchItems` cleared the accumulated ranges with `setPendingRanges([])`, and `pendingRanges` is a dependency of the effect that calls `fetchItems`. A fresh `[]` is a new identity every time, so the effect re-ran, re-armed the 500ms throttle, and cleared again — a self-sustaining render loop that ran as fast as the throttle allowed, with no user input, for as long as the gallery grid was mounted. Clear with the shared stable `EMPTY_ARRAY` reference instead, so React bails out rather than re-running the effect. The queue variant returned early — before clearing — when nothing was uncached, which happened to prevent the loop while everything was cached, at the cost of letting ranges accumulate for the lifetime of the list and growing the scan on every pass. It now clears on both paths, with the stable reference doing the work of stopping the loop. Retry on failure explicitly, because the loop was doing it accidentally. These bulk fetches are the only fetcher for their rows: `ImageAtPosition` and `QueueItemAtPosition` both consume the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not fetch for itself, and images have no retry affordance. Without this, a transient failure would leave placeholders until the user happened to scroll, where before the loop re-tried until it succeeded. Co-Authored-By: Claude Opus 5 (1M context) --- .../hooks/useRangeBasedImageFetching.ts | 22 ++++++++++++--- .../hooks/useRangeBasedQueueItemFetching.ts | 27 +++++++++++++++---- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index eab38776e5b..6264189747f 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -1,3 +1,4 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; import { isVideoName } from 'features/gallery/store/types'; import { useCallback, useEffect, useState } from 'react'; @@ -51,7 +52,7 @@ export const useRangeBasedImageFetching = ({ const store = useAppStore(); const [getImageDTOsByNames] = useGetImageDTOsByNamesMutation(); const [lastRange, setLastRange] = useState(null); - const [pendingRanges, setPendingRanges] = useState([]); + const [pendingRanges, setPendingRanges] = useState(EMPTY_ARRAY); const fetchItems = useCallback( (ranges: ListRange[], allNames: string[]) => { @@ -64,7 +65,16 @@ export const useRangeBasedImageFetching = ({ const cachedImageNames = imagesApi.util.selectCachedArgsForQuery(state, 'getImageDTO'); const uncachedImageNames = getUncachedNames(allNames, cachedImageNames, ranges).filter((n) => !isVideoName(n)); if (uncachedImageNames.length > 0) { - getImageDTOsByNames({ image_names: uncachedImageNames }); + getImageDTOsByNames({ image_names: uncachedImageNames }) + .unwrap() + .catch(() => { + // This bulk fetch is the ONLY fetcher for these rows: `ImageAtPosition` consumes the + // cache with `skip: isUninitialized`, so a row whose DTO never arrived does not fetch + // for itself, and images (unlike videos) have no retry affordance. Put the ranges back + // so the effect re-runs and tries again — otherwise a transient failure leaves grey + // placeholders until the user happens to scroll. The throttle bounds the retry rate. + setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + }); } // Videos — fetch one at a time (no batch endpoint yet). Each `initiate()` is a no-op for @@ -77,7 +87,13 @@ export const useRangeBasedImageFetching = ({ store.dispatch(videosApi.endpoints.getVideoDTO.initiate(videoName, getVideoPrefetchOptions())); } - setPendingRanges([]); + // Clear with a stable reference. `pendingRanges` is a dependency of the effect that + // calls this function, so a fresh `[]` — a new identity every time — re-runs the + // effect, which re-arms the throttle, which calls this again: a self-sustaining + // render loop, running as fast as the throttle allows, for as long as the grid is + // mounted and with no user input. Setting state to the value it already holds makes + // React bail out instead. + setPendingRanges(EMPTY_ARRAY); }, [enabled, getImageDTOsByNames, store] ); diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index b2d4c4ac813..33697875542 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -1,3 +1,4 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; import { useCallback, useEffect, useState } from 'react'; import type { ListRange } from 'react-virtuoso'; @@ -41,7 +42,7 @@ export const useRangeBasedQueueItemFetching = ({ const store = useAppStore(); const [getQueueItemDTOsByItemIds] = useGetQueueItemDTOsByItemIdsMutation(); const [lastRange, setLastRange] = useState(null); - const [pendingRanges, setPendingRanges] = useState([]); + const [pendingRanges, setPendingRanges] = useState(EMPTY_ARRAY); const fetchQueueItems = useCallback( (ranges: ListRange[], itemIds: number[]) => { @@ -50,11 +51,27 @@ export const useRangeBasedQueueItemFetching = ({ } const cachedItemIds = queueApi.util.selectCachedArgsForQuery(store.getState(), 'getQueueItem'); const uncachedItemIds = getUncachedItemIds(itemIds, cachedItemIds, ranges); - if (uncachedItemIds.length === 0) { - return; + if (uncachedItemIds.length > 0) { + getQueueItemDTOsByItemIds({ item_ids: uncachedItemIds }) + .unwrap() + .catch(() => { + // This bulk fetch is the ONLY fetcher for these rows: `QueueItemAtPosition` consumes + // the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not + // fetch for itself. Put the ranges back so the effect re-runs and tries again — + // otherwise a transient failure leaves placeholders until the user happens to scroll. + setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + }); } - getQueueItemDTOsByItemIds({ item_ids: uncachedItemIds }); - setPendingRanges([]); + // Clear unconditionally. Returning early without clearing (the previous behaviour when + // everything was already cached) let ranges accumulate for the lifetime of the list, + // growing the scan on every subsequent pass. + // + // Clear with a stable reference. `pendingRanges` is a dependency of the effect that calls + // this function, so a fresh `[]` — a new identity every time — re-runs the effect, which + // re-arms the throttle, which calls this again. The old early return happened to prevent + // that while everything was cached, so the loop only ran while items were genuinely + // uncached; clearing on both paths means the stable reference is now what stops it. + setPendingRanges(EMPTY_ARRAY); }, [enabled, getQueueItemDTOsByItemIds, store] ); From 392e5ae5548af74beeb91ffbfffd81a04608ffc7 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 2 Aug 2026 15:53:30 -0400 Subject: [PATCH 2/2] test(ui): regression tests for the range-based fetching render loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Render both hooks with React act + fake timers in a happy-dom environment (scoped per-file via a @vitest-environment docblock; happy-dom is the only new dev dependency) and mock only the thin API-endpoint modules, so the tests exercise the real state/effect/throttle cycle the fix changed. Covered per hook: - a reported range fetches its uncached items once, then renders and fetches both go quiet (the pre-fix loop re-rendered every throttle window forever, and in the gallery hook ran from mount even with nothing to fetch) - items that never land in the cache (deleted image, multiuser ownership filter) are not re-requested indefinitely — bounded, then quiet, where the pre-fix loop was a permanent one-request-per-window stream - a failed bulk fetch is retried until it succeeds, then goes quiet — the explicit replacement for the retry the loop provided accidentally - every range reported within a throttle window is fetched, not just the last (the pendingRanges accumulation onRangeChanged exists for) - handled ranges are dropped, not accumulated: an item evicted from a long-handled range is not re-requested by later passes (the queue hook's pre-fix early return without clearing regressed exactly this) - new ranges after settling still fetch, and enabled=false fetches nothing The time-advance helper steps in small increments with an act flush per step; a single long advance would defer effect re-runs to the end of the act scope and break the very feedback cycle (state update -> effect -> throttle -> fetch) the suite exists to detect. Mutation-verified: reverting the EMPTY_ARRAY clears, restoring the queue hook's early return, dropping onRangeChanged's accumulation, or neutering the retry catch each makes at least one test fail; all pass with the fix in place. Co-Authored-By: Claude Fable 5 --- invokeai/frontend/web/package.json | 1 + invokeai/frontend/web/pnpm-lock.yaml | 77 +++++- .../hooks/useRangeBasedImageFetching.test.ts | 248 +++++++++++++++++- .../useRangeBasedQueueItemFetching.test.ts | 223 ++++++++++++++++ 4 files changed, 542 insertions(+), 7 deletions(-) create mode 100644 invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts diff --git a/invokeai/frontend/web/package.json b/invokeai/frontend/web/package.json index 6c7ea3f65ca..2ba037653e5 100644 --- a/invokeai/frontend/web/package.json +++ b/invokeai/frontend/web/package.json @@ -142,6 +142,7 @@ "eslint-plugin-storybook": "^10.3.6", "eslint-plugin-unused-imports": "^4.4.1", "globals": "^16.5.0", + "happy-dom": "^20.11.1", "knip": "^5.77.4", "magic-string": "^0.30.21", "openapi-types": "^12.1.3", diff --git a/invokeai/frontend/web/pnpm-lock.yaml b/invokeai/frontend/web/pnpm-lock.yaml index 4901ad00405..b45e368e497 100644 --- a/invokeai/frontend/web/pnpm-lock.yaml +++ b/invokeai/frontend/web/pnpm-lock.yaml @@ -297,6 +297,9 @@ importers: globals: specifier: ^16.5.0 version: 16.5.0 + happy-dom: + specifier: ^20.11.1 + version: 20.11.1 knip: specifier: ^5.77.4 version: 5.77.4(@types/node@22.19.3)(typescript@5.9.3) @@ -338,7 +341,7 @@ importers: version: 1.8.1(eslint@9.39.2(jiti@2.6.1))(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) vitest: specifier: ^4.1.5 - version: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + version: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) packages: @@ -1969,6 +1972,12 @@ packages: '@types/uuid@10.0.0': resolution: {integrity: sha512-7gqG38EyHgyP1S+7+xomFtL+ZNHcKv6DwNaCZmJmo1vgMugyF3TCnXVg4t1uk89mLNwnLtnY3TpOpCOyp1/xHQ==} + '@types/whatwg-mimetype@3.0.2': + resolution: {integrity: sha512-c2AKvDT8ToxLIOUlN51gTiHXflsfIFisS4pO7pDPoKouJCESkhZnEy623gwP9laCy5lnLDAw1vAzu2vM2YLOrA==} + + '@types/ws@8.18.1': + resolution: {integrity: sha512-ThVF6DCVhA8kUGy+aazFQ4kXQ7E1Ty7A3ypFOe0IcJV8O/M511G99AW24irKrW56Wt44yG9+ij8FaqoBGkuBXg==} + '@typescript-eslint/eslint-plugin@8.59.2': resolution: {integrity: sha512-j/bwmkBvHUtPNxzuWe5z6BEk3q54YRyGlBXkSsmfoih7zNrBvl5A9A98anlp/7JbyZcWIJ8KXo/3Tq/DjFLtuQ==} engines: {node: ^18.18.0 || ^20.9.0 || >=21.1.0} @@ -2331,6 +2340,10 @@ packages: engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7} hasBin: true + buffer-image-size@0.6.4: + resolution: {integrity: sha512-nEh+kZOPY1w+gcCMobZ6ETUp9WfibndnosbpwB1iJk/8Gt5ZF2bhS6+B6bPYz424KtwsR6Rflc3tCz1/ghX2dQ==} + engines: {node: '>=4.0'} + buffer@5.7.1: resolution: {integrity: sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ==} @@ -2660,6 +2673,10 @@ packages: resolution: {integrity: sha512-HqD3yTBfnBxIrbnM1DoD6Pcq8NECnh8d4As1Qgh0z5Gg3jRRIqijury0CL3ghu/edArpUYiYqQiDUQBIs4np3Q==} engines: {node: '>=10.0.0'} + entities@7.0.1: + resolution: {integrity: sha512-TWrgLOFUQTH994YUyl1yT4uyavY5nNB5muff+RtWaqNVCAK408b5ZnnbNAUEWLTCpum9w6arT70i1XdQ4UeOPA==} + engines: {node: '>=0.12'} + error-ex@1.3.4: resolution: {integrity: sha512-sqQamAnR14VgCr1A618A3sGrygcpK+HEbenA/HiEAkkUwcZIIB/tgWqHFxWgOyDh4nB4JCRimh79dR5Ywc9MDQ==} @@ -3070,6 +3087,10 @@ packages: graceful-fs@4.2.11: resolution: {integrity: sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==} + happy-dom@20.11.1: + resolution: {integrity: sha512-XSt8tMzbW9ymE7687xztkO1ckR7qJNQ3LywY9vlYGhGi3zXrGBHuUo2Cl1ztZaICW+1eAGdkLbj6iwVqDT33kg==} + engines: {node: '>=20.0.0'} + has-bigints@1.1.0: resolution: {integrity: sha512-R3pbpkcIqv2Pm3dUwgjclDRVmWpTJW2DcMzcIhEXEx1oh/CEMObMm3KLmRJOdvhM7o4uQBnwr8pzRK2sJWIqfg==} engines: {node: '>= 0.4'} @@ -4854,6 +4875,10 @@ packages: webpack-virtual-modules@0.6.2: resolution: {integrity: sha512-66/V2i5hQanC51vBQKPH4aI8NMAcBW59FVBs+rC7eGHupMyfn34q7rZIE+ETlJ+XTevqfUhVVBgSUNSW2flEUQ==} + whatwg-mimetype@3.0.0: + resolution: {integrity: sha512-nt+N2dzIutVRxARx1nghPKGv1xHikU7HKdfafKkLNLindmPU/ch3U31NOCGGA/dmPcmb1VlofO0vnKAcsm0o/Q==} + engines: {node: '>=12'} + whatwg-url@5.0.0: resolution: {integrity: sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==} @@ -4919,6 +4944,18 @@ packages: utf-8-validate: optional: true + ws@8.21.1: + resolution: {integrity: sha512-+0NTnW77fFN/DjQi6k/Sq/Yvk4Sgajw7urW8V+asjXnRgDs9gyGkdb7EzgfhA4goXsRIZKE28fzIXBHEzhuiWw==} + engines: {node: '>=10.0.0'} + peerDependencies: + bufferutil: ^4.0.1 + utf-8-validate: '>=5.0.2' + peerDependenciesMeta: + bufferutil: + optional: true + utf-8-validate: + optional: true + wsl-utils@0.1.0: resolution: {integrity: sha512-h3Fbisa2nKGPxCpm89Hk33lBLsnaGBvctQopaBSOW/uIs6FTe1ATyAnKFJrzVs9vpGdsTe73WF3V4lIsk4Gacw==} engines: {node: '>=18'} @@ -6497,7 +6534,7 @@ snapshots: '@testing-library/dom@10.4.0': dependencies: - '@babel/code-frame': 7.29.0 + '@babel/code-frame': 7.29.7 '@babel/runtime': 7.29.2 '@types/aria-query': 5.0.4 aria-query: 5.3.0 @@ -6635,6 +6672,12 @@ snapshots: '@types/uuid@10.0.0': {} + '@types/whatwg-mimetype@3.0.2': {} + + '@types/ws@8.18.1': + dependencies: + '@types/node': 22.19.3 + '@typescript-eslint/eslint-plugin@8.59.2(@typescript-eslint/parser@8.59.2(eslint@9.39.2(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.2(jiti@2.6.1))(typescript@5.9.3)': dependencies: '@eslint-community/regexpp': 4.12.2 @@ -6797,7 +6840,7 @@ snapshots: obug: 2.1.1 std-env: 4.1.0 tinyrainbow: 3.1.0 - vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) '@vitest/expect@3.2.4': dependencies: @@ -6859,7 +6902,7 @@ snapshots: sirv: 3.0.2 tinyglobby: 0.2.16 tinyrainbow: 3.1.0 - vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) '@vitest/utils@3.2.4': dependencies: @@ -7110,6 +7153,10 @@ snapshots: node-releases: 2.0.38 update-browserslist-db: 1.2.3(browserslist@4.28.2) + buffer-image-size@0.6.4: + dependencies: + '@types/node': 22.19.3 + buffer@5.7.1: dependencies: base64-js: 1.5.1 @@ -7445,6 +7492,8 @@ snapshots: engine.io-parser@5.2.3: {} + entities@7.0.1: {} + error-ex@1.3.4: dependencies: is-arrayish: 0.2.1 @@ -7992,6 +8041,19 @@ snapshots: graceful-fs@4.2.11: {} + happy-dom@20.11.1: + dependencies: + '@types/node': 22.19.3 + '@types/whatwg-mimetype': 3.0.2 + '@types/ws': 8.18.1 + buffer-image-size: 0.6.4 + entities: 7.0.1 + whatwg-mimetype: 3.0.0 + ws: 8.21.1 + transitivePeerDependencies: + - bufferutil + - utf-8-validate + has-bigints@1.1.0: {} has-flag@4.0.0: {} @@ -9744,7 +9806,7 @@ snapshots: fsevents: 2.3.3 jiti: 2.6.1 - vitest@4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)): + vitest@4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)): dependencies: '@vitest/expect': 4.1.5 '@vitest/mocker': 4.1.5(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) @@ -9770,6 +9832,7 @@ snapshots: '@types/node': 22.19.3 '@vitest/coverage-v8': 4.1.5(vitest@4.1.5) '@vitest/ui': 4.1.5(vitest@4.1.5) + happy-dom: 20.11.1 transitivePeerDependencies: - msw @@ -9785,6 +9848,8 @@ snapshots: webpack-virtual-modules@0.6.2: {} + whatwg-mimetype@3.0.0: {} + whatwg-url@5.0.0: dependencies: tr46: 0.0.3 @@ -9858,6 +9923,8 @@ snapshots: ws@8.20.0: {} + ws@8.21.1: {} + wsl-utils@0.1.0: dependencies: is-wsl: 3.1.1 diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts index 6cec16aa043..8194c5c965c 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts @@ -1,6 +1,250 @@ -import { describe, expect, it } from 'vitest'; +// @vitest-environment happy-dom +import { act, createElement, type FC } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { ListRange } from 'react-virtuoso'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { getVideoPrefetchOptions, hasCachedVideoDTO } from './useRangeBasedImageFetching'; +import { getVideoPrefetchOptions, hasCachedVideoDTO, useRangeBasedImageFetching } from './useRangeBasedImageFetching'; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const mocks = vi.hoisted(() => ({ + // Args of every getImageDTOsByNames call, in order. + imageFetches: [] as string[][], + // Names with a getImageDTO cache entry, as reported by selectCachedArgsForQuery. + cachedImageNames: [] as string[], + // When true, a successful fetch upserts the requested names into the cache, like + // getImageDTOsByNames.onQueryStarted does. When false, requested names never land in the + // cache — the deleted-image / multiuser-filtered case that drove the pre-fix request stream. + cacheLands: true, + // When true, the mutation rejects, like a backend restart or a 502 from a reverse proxy. + failFetches: false, +})); + +vi.mock('app/store/storeHooks', () => { + const store = { getState: () => ({}), dispatch: () => undefined }; + return { useAppStore: () => store }; +}); + +vi.mock('features/gallery/store/types', () => ({ + isVideoName: (name: string) => name.endsWith('.mp4'), +})); + +vi.mock('services/api/endpoints/images', () => { + const trigger = (arg: { image_names: string[] }) => { + mocks.imageFetches.push(arg.image_names); + // Like the real mutation: onQueryStarted upserts when the request fulfills, whether or not + // the caller unwraps, and only the promise returned by unwrap() surfaces the rejection. + const settled = mocks.failFetches + ? Promise.reject(new Error('fetch failed')) + : Promise.resolve().then(() => { + if (mocks.cacheLands) { + mocks.cachedImageNames.push(...arg.image_names); + } + return []; + }); + settled.catch(() => undefined); + return { unwrap: () => settled.then((r) => r) }; + }; + // RTK Query's mutation trigger is referentially stable across renders; the hook's fetchItems + // callback (and therefore its throttle and effect) depend on that. + const result = [trigger]; + return { + imagesApi: { util: { selectCachedArgsForQuery: () => mocks.cachedImageNames } }, + useGetImageDTOsByNamesMutation: () => result, + }; +}); + +vi.mock('services/api/endpoints/videos', () => ({ + videosApi: { + util: { selectCachedArgsForQuery: () => [] }, + endpoints: { getVideoDTO: { select: () => () => ({ data: undefined }), initiate: () => ({ type: 'noop' }) } }, + }, +})); + +const IMAGE_NAMES = ['a.png', 'b.png', 'c.png']; +const THROTTLE_MS = 500; + +describe('useRangeBasedImageFetching', () => { + let root: Root | null = null; + let renderCount = 0; + let hookReturn: ReturnType; + + const renderHook = (imageNames: string[], enabled: boolean) => { + const Harness: FC = () => { + renderCount++; + hookReturn = useRangeBasedImageFetching({ imageNames, enabled }); + return null; + }; + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness)); + }); + }; + + const scrollTo = (range: ListRange) => { + act(() => { + hookReturn.onRangeChanged(range); + }); + }; + + // Advance fake time in small steps, flushing React work (renders + effects) between steps. A + // single long advance would defer all effect re-runs to the end of the act scope, which breaks + // the feedback cycle this suite exists to detect: state update -> effect -> throttle -> fetch -> + // state update. Stepping mimics real event-loop turns, letting a loop sustain itself if the + // code allows one. + const advance = async (ms: number) => { + const step = 250; + for (let elapsed = 0; elapsed < ms; elapsed += step) { + await act(async () => { + await vi.advanceTimersByTimeAsync(step); + }); + } + }; + + beforeEach(() => { + vi.useFakeTimers(); + mocks.imageFetches = []; + mocks.cachedImageNames = []; + mocks.cacheLands = true; + mocks.failFetches = false; + renderCount = 0; + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + vi.useRealTimers(); + }); + + it('fetches uncached names for a reported range, then goes quiet', async () => { + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + + expect(mocks.imageFetches).toEqual([IMAGE_NAMES]); + + // Regression: clearing pendingRanges with a fresh `[]` (a new identity every time) re-ran the + // effect, re-armed the throttle, and cleared again — a self-sustaining render loop that + // re-rendered every ~500ms for as long as the grid was mounted, with no user input. Once the + // range has been handled and the throttle has drained, both renders and fetches must stop. + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.imageFetches).toEqual([IMAGE_NAMES]); + }); + + it('does not loop even while the grid is mounted with nothing to fetch', async () => { + // Pre-fix, the loop ran from mount even with no ranges reported, because the clear was + // unconditional and every pass installed a new [] identity. + renderHook(IMAGE_NAMES, true); + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.imageFetches).toEqual([]); + }); + + it('stops re-requesting names that never land in the cache', async () => { + // onQueryStarted upserts only the DTOs the server actually returned, so a requested name that + // comes back missing (deleted image, multiuser ownership filter) never gets a cache entry. + // Pre-fix, the render loop re-requested such names every ~500ms, forever. + mocks.cacheLands = false; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + + // The range-change pass fetches once, and clearing pendingRanges ([range] -> EMPTY_ARRAY) is a + // real state change, so one follow-up pass may re-check the cache and re-request the + // still-missing names. After that the state is stable and the stream must stop — pre-fix it + // continued at one request per throttle window, forever. + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(1); + expect(mocks.imageFetches.length).toBeLessThanOrEqual(2); + const settledFetches = mocks.imageFetches.length; + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches.length).toBe(settledFetches); + }); + + it('retries a failed fetch until it succeeds, then goes quiet', async () => { + // The pre-fix loop was also an accidental retry, and this bulk fetch is the only fetcher for + // these rows (ImageAtPosition subscribes with `skip: isUninitialized`). Without an explicit + // retry, a transient failure would leave grey placeholders until the user happens to scroll. + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + + // The catch-driven retry produces a fetch per throttle window. Without it, clearing + // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at + // two — three or more requires the catch handler restoring the ranges. + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(3); + expect(mocks.cachedImageNames).toEqual([]); + + mocks.failFetches = false; + await advance(THROTTLE_MS * 4); + expect(mocks.cachedImageNames).toEqual(IMAGE_NAMES); + + const fetchesAfterRecovery = mocks.imageFetches.length; + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches.length).toBe(fetchesAfterRecovery); + expect(renderCount).toBe(settledRenders); + }); + + it('still fetches for new ranges after settling', async () => { + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches).toEqual([['a.png', 'b.png', 'c.png']]); + + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([ + ['a.png', 'b.png', 'c.png'], + ['d.png', 'e.png', 'f.png'], + ]); + }); + + it('fetches every range reported within a throttle window, not just the last', async () => { + // onRangeChanged accumulates ranges into pendingRanges precisely so that ranges reported + // mid-window are not dropped when the trailing invocation only sees the latest call's args. + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']]); + }); + + it('drops handled ranges instead of accumulating them', async () => { + // A handled range must not be re-scanned by later passes. Pre-fix, the queue variant of this + // hook returned early without clearing when everything was cached, so ranges accumulated for + // the lifetime of the list and a later pass would re-request an item evicted from a range + // handled long ago. + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + mocks.cachedImageNames = ['a.png', 'b.png', 'c.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches).toEqual([]); + + mocks.cachedImageNames = ['a.png', 'c.png']; + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([['d.png', 'e.png', 'f.png']]); + }); + + it('does not fetch when disabled', async () => { + renderHook(IMAGE_NAMES, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + expect(mocks.imageFetches).toEqual([]); + }); +}); describe('video range prefetch', () => { it('does not retain an RTK Query subscription', () => { diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts new file mode 100644 index 00000000000..fb40bd55d35 --- /dev/null +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -0,0 +1,223 @@ +// @vitest-environment happy-dom +import { act, createElement, type FC } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { ListRange } from 'react-virtuoso'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useRangeBasedQueueItemFetching } from './useRangeBasedQueueItemFetching'; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const mocks = vi.hoisted(() => ({ + // Args of every getQueueItemDTOsByItemIds call, in order. + queueFetches: [] as number[][], + // Item ids with a getQueueItem cache entry, as reported by selectCachedArgsForQuery. + cachedItemIds: [] as number[], + // When true, a successful fetch upserts the requested ids into the cache, like the mutation's + // onQueryStarted does. When false, requested ids never land in the cache. + cacheLands: true, + // When true, the mutation rejects, like a backend restart or a 502 from a reverse proxy. + failFetches: false, +})); + +vi.mock('app/store/storeHooks', () => { + const store = { getState: () => ({}), dispatch: () => undefined }; + return { useAppStore: () => store }; +}); + +vi.mock('services/api/endpoints/queue', () => { + const trigger = (arg: { item_ids: number[] }) => { + mocks.queueFetches.push(arg.item_ids); + // Like the real mutation: onQueryStarted upserts when the request fulfills, whether or not + // the caller unwraps, and only the promise returned by unwrap() surfaces the rejection. + const settled = mocks.failFetches + ? Promise.reject(new Error('fetch failed')) + : Promise.resolve().then(() => { + if (mocks.cacheLands) { + mocks.cachedItemIds.push(...arg.item_ids); + } + return []; + }); + settled.catch(() => undefined); + return { unwrap: () => settled.then((r) => r) }; + }; + // RTK Query's mutation trigger is referentially stable across renders; the hook's + // fetchQueueItems callback (and therefore its throttle and effect) depend on that. + const result = [trigger]; + return { + queueApi: { util: { selectCachedArgsForQuery: () => mocks.cachedItemIds } }, + useGetQueueItemDTOsByItemIdsMutation: () => result, + }; +}); + +const ITEM_IDS = [1, 2, 3]; +const THROTTLE_MS = 500; + +describe('useRangeBasedQueueItemFetching', () => { + let root: Root | null = null; + let renderCount = 0; + let hookReturn: ReturnType; + + const renderHook = (itemIds: number[], enabled: boolean) => { + const Harness: FC = () => { + renderCount++; + hookReturn = useRangeBasedQueueItemFetching({ itemIds, enabled }); + return null; + }; + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness)); + }); + }; + + const scrollTo = (range: ListRange) => { + act(() => { + hookReturn.onRangeChanged(range); + }); + }; + + // Advance fake time in small steps, flushing React work (renders + effects) between steps. A + // single long advance would defer all effect re-runs to the end of the act scope, which breaks + // the feedback cycle this suite exists to detect: state update -> effect -> throttle -> fetch -> + // state update. Stepping mimics real event-loop turns, letting a loop sustain itself if the + // code allows one. + const advance = async (ms: number) => { + const step = 250; + for (let elapsed = 0; elapsed < ms; elapsed += step) { + await act(async () => { + await vi.advanceTimersByTimeAsync(step); + }); + } + }; + + beforeEach(() => { + vi.useFakeTimers(); + mocks.queueFetches = []; + mocks.cachedItemIds = []; + mocks.cacheLands = true; + mocks.failFetches = false; + renderCount = 0; + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + vi.useRealTimers(); + }); + + it('fetches uncached items for a reported range, then goes quiet', async () => { + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + + expect(mocks.queueFetches).toEqual([ITEM_IDS]); + + // Regression: clearing pendingRanges with a fresh `[]` (a new identity every time) re-ran the + // effect, re-armed the throttle, and cleared again — a self-sustaining render loop. Once the + // range has been handled and the throttle has drained, both renders and fetches must stop. + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.queueFetches).toEqual([ITEM_IDS]); + }); + + it('stops re-requesting items that never land in the cache', async () => { + // A requested id the server does not return never gets a getQueueItem cache entry, so it is + // uncached on every pass. Pre-fix, that sustained the loop: the list re-requested such ids + // every ~500ms for as long as it was mounted. + mocks.cacheLands = false; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + + // The range-change pass fetches once, and clearing pendingRanges ([range] -> EMPTY_ARRAY) is a + // real state change, so one follow-up pass may re-check the cache and re-request the + // still-missing ids. After that the state is stable and the stream must stop — pre-fix it + // continued at one request per throttle window, forever. + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(1); + expect(mocks.queueFetches.length).toBeLessThanOrEqual(2); + const settledFetches = mocks.queueFetches.length; + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches.length).toBe(settledFetches); + }); + + it('retries a failed fetch until it succeeds, then goes quiet', async () => { + // This bulk fetch is the only fetcher for these rows (QueueItemAtPosition subscribes with + // `skip: isUninitialized`), so a transient failure must be retried or the placeholders stay + // empty until the user happens to scroll. + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + + // The catch-driven retry produces a fetch per throttle window. Without it, clearing + // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at + // two — three or more requires the catch handler restoring the ranges. + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(3); + expect(mocks.cachedItemIds).toEqual([]); + + mocks.failFetches = false; + await advance(THROTTLE_MS * 4); + expect(mocks.cachedItemIds).toEqual(ITEM_IDS); + + const fetchesAfterRecovery = mocks.queueFetches.length; + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches.length).toBe(fetchesAfterRecovery); + expect(renderCount).toBe(settledRenders); + }); + + it('still fetches for new ranges after settling', async () => { + const itemIds = [1, 2, 3, 4, 5, 6]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches).toEqual([[1, 2, 3]]); + + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([ + [1, 2, 3], + [4, 5, 6], + ]); + }); + + it('fetches every range reported within a throttle window, not just the last', async () => { + // onRangeChanged accumulates ranges into pendingRanges precisely so that ranges reported + // mid-window are not dropped when the trailing invocation only sees the latest call's args. + const itemIds = [1, 2, 3, 4, 5, 6]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([[1, 2, 3, 4, 5, 6]]); + }); + + it('drops handled ranges instead of accumulating them', async () => { + // A handled range must not be re-scanned by later passes. Pre-fix, this hook returned early + // without clearing when everything was cached, so ranges accumulated for the lifetime of the + // list and a later pass would re-request an item evicted from a range handled long ago. + const itemIds = [1, 2, 3, 4, 5, 6]; + mocks.cachedItemIds = [1, 2, 3]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches).toEqual([]); + + mocks.cachedItemIds = [1, 3]; + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([[4, 5, 6]]); + }); + + it('does not fetch when disabled', async () => { + renderHook(ITEM_IDS, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + expect(mocks.queueFetches).toEqual([]); + }); +});