From eebcefbcf0e5ba0cfa4142f23fcbb10f07c9347f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 1 Aug 2026 18:29:21 -0400 Subject: [PATCH 1/8] fix(ui): resolve the viewer preview on the thumbnail and stop it sticking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The image viewer holds the last progress preview on screen until the final image's onLoad fires. Two problems with that. The reveal was gated on a preload of imageDTO.image_url — the full-resolution PNG — so on a slow connection the stale latent preview stayed up for the entire multi-megabyte download. A 256px thumbnail is already generated for every image and is typically higher resolution than the preview it replaces. Gate on that instead; DndImage renders it via Chakra's fallbackSrc and swaps the full image in, in place, once it arrives. The preload also used the raw URL while DndImage requests useMediaUrl(...), which appends ?media_cookie_version=N. Different key, so the bytes were fetched twice (measured: 2 requests mismatched vs 1 matched). Route the preload through useMediaUrl so it is byte-identical. The reuse is the document's list of available images, keyed by URL rather than the HTTP cache, so it still holds in multiuser mode where images are served Cache-Control: private, no-store. Separately, the viewer's progress atoms are distinct stores from the global ones in services/events/stores, and only the latter were reset on socket lifecycle transitions. socket.io has no event replay, so a drop spanning the terminal queue_item_status_changed loses that event permanently and nothing is left to clear the opaque overlay covering the finished image — the reported "backgrounded the tab, came back, only a reload fixes it". Reset the viewer's atoms on connect/connect_error/disconnect too, matching setEventListeners. onLoadImage is not a guaranteed callback in any case: Chakra reports a failed load as onError, useImage only re-runs when src changes, the load can beat the terminal event, and an all-intermediate item never changes the selection. So the deferred clear also gets a backstop deadline. The armed flag and its timer live together in createDeferredClear — as separate state, a path that reset the flag but leaked the timer let a deadline outlive the generation that armed it and blank a later one's live preview. The backstop does not clear while other sessions still have previews, since nulling $progressImage tears down the whole overlay including multi-GPU tiles, and the reconnect reset only replaces the map when it holds something, because connect_error fires once per reconnection attempt. The terminal-status policy moves to a pure getTerminalProgressAction so the branchy decision is testable without a socket or a React tree. Co-Authored-By: Claude Opus 5 (1M context) --- .../ImageViewer/CurrentImagePreview.tsx | 25 ++- .../components/ImageViewer/context.test.ts | 65 ++++++++ .../components/ImageViewer/context.tsx | 147 ++++++++++++----- .../progressImageResolution.test.ts | 150 ++++++++++++++++++ .../ImageViewer/progressImageResolution.ts | 126 +++++++++++++++ 5 files changed, 468 insertions(+), 45 deletions(-) create mode 100644 invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts create mode 100644 invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts create mode 100644 invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx index 1978a7fc1ab..06550899a2d 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -1,6 +1,7 @@ import { Box, Flex } from '@invoke-ai/ui-library'; import { useStore } from '@nanostores/react'; import { useAppSelector } from 'app/store/storeHooks'; +import { useMediaUrl } from 'features/auth/store/mediaCookieRefresh'; import { CanvasAlertsInvocationProgress } from 'features/controlLayers/components/CanvasAlerts/CanvasAlertsInvocationProgress'; import { DndImage } from 'features/dnd/DndImage'; import ImageMetadataViewer from 'features/gallery/components/ImageMetadataViewer/ImageMetadataViewer'; @@ -48,6 +49,20 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu const previousRenderedImageNameRef = useRef(null); const selectedImageRevealTimeoutId = useRef(0); + // The reveal gate below deliberately preloads the *thumbnail*, not the full-resolution image. The + // progress overlay covers this element until onLoadImage fires, so gating on the multi-megabyte + // `/full` response would hold a stale latent preview on screen for that entire download on a slow + // connection. The 256px thumbnail is roughly 100x smaller and is typically higher resolution than + // the preview it replaces; DndImage renders it via Chakra's `fallbackSrc` and swaps the full image + // in, in place, once that finishes loading. + // + // The URL must go through useMediaUrl so it is byte-identical to the one DndImage requests. The + // media cookie version is a query parameter, so a mismatch is a different key and the bytes are + // fetched twice (measured: 2 requests mismatched vs 1 matched). Note the reuse here is the + // document's list of available images, which is keyed by URL and is not the HTTP cache — it still + // holds in multiuser mode, where images are served `Cache-Control: private, no-store`. + const previewSrc = useMediaUrl(imageDTO?.thumbnail_url); + useEffect(() => { if (!selectedImageName) { setImageToRender(null); @@ -65,9 +80,13 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu return; } setImageToRender(imageDTO); + // Resolve the progress overlay as soon as the thumbnail settles — on success *or* error. + // Relying on DndImage's onLoad alone leaves the overlay stuck whenever the image fails to + // load, because Chakra reports that as onError instead. + onLoadImage(); }; - if (typeof window === 'undefined') { + if (typeof window === 'undefined' || !previewSrc) { onReady(); return; } @@ -76,7 +95,7 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu preloader.onload = onReady; preloader.onerror = onReady; - preloader.src = imageDTO.image_url; + preloader.src = previewSrc; if (preloader.complete) { onReady(); @@ -87,7 +106,7 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu preloader.onload = null; preloader.onerror = null; }; - }, [imageDTO, imageToRender?.image_name, selectedImageName]); + }, [imageDTO, imageToRender?.image_name, onLoadImage, previewSrc, selectedImageName]); const hasProgressImage = progressImage !== null; diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts new file mode 100644 index 00000000000..c2a7dc8b597 --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts @@ -0,0 +1,65 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +import { describe, expect, it } from 'vitest'; + +const read = (file: string) => readFileSync(fileURLToPath(new URL(file, import.meta.url)), 'utf8'); + +// The behaviour of the deferred clear itself is covered by real tests in +// progressImageResolution.test.ts. These are wiring checks only — this directory has no DOM test +// environment, so the provider cannot be mounted. They assert that context.tsx routes through the +// tested unit rather than reimplementing the state inline, which is what previously allowed the +// armed flag and its timer to drift apart. +describe('ImageViewer progress image wiring', () => { + const context = read('./context.tsx'); + const currentImagePreview = read('./CurrentImagePreview.tsx'); + + it('resets the viewer progress atoms on every socket lifecycle transition', () => { + // socket.io has no event replay, so a drop spanning the terminal queue_item_status_changed + // loses that event permanently. Without these the overlay covers the finished image until the + // page is reloaded. setEventListeners already does the same for the global progress stores. + for (const event of ['connect', 'connect_error', 'disconnect']) { + expect(context).toContain(`socket.on('${event}', onSocketLifecycleChange)`); + expect(context).toContain(`socket.off('${event}', onSocketLifecycleChange)`); + } + }); + + it('keeps the armed flag and its backstop timer in one owned unit', () => { + // Both must come from createDeferredClear. A local boolean ref plus a separate timeout id is + // exactly the shape that let a stale timer outlive the generation that armed it. + expect(context).toContain('createDeferredClear()'); + expect(context).toContain('deferredClear.arm(onResolveDeadline)'); + expect(context).toContain('deferredClear.isArmed()'); + expect(context).not.toContain('shouldClearProgressImageOnLoadRef'); + expect(context).not.toContain('setTimeout'); + }); + + it('supersedes a pending backstop when a new progress event arrives', () => { + // Otherwise: item N completes and arms the backstop, its final image never loads, the user + // starts item N+1, and N's deadline fires mid-generation and blanks N+1's live preview. + const progressHandler = context.slice( + context.indexOf('const onInvocationProgress ='), + context.indexOf("socket.on('invocation_progress'") + ); + expect(progressHandler).toContain('disarmDeferredClear()'); + }); + + it('gates the viewer reveal on the thumbnail rather than the full-resolution image', () => { + // Gating on `/full` holds a stale latent preview on screen for the whole multi-megabyte + // download on a slow connection. + expect(currentImagePreview).toContain('useMediaUrl(imageDTO?.thumbnail_url)'); + expect(currentImagePreview).toContain('preloader.src = previewSrc'); + expect(currentImagePreview).not.toMatch(/preloader\.src\s*=\s*imageDTO\.image_url/); + }); + + it('clears the progress overlay when the preload settles, including on error', () => { + // Chakra reports a failed load as onError, not onLoad, so DndImage's onLoad alone is not + // enough to guarantee the overlay is ever cleared. + expect(currentImagePreview).toContain('preloader.onerror = onReady'); + const onReady = currentImagePreview.slice( + currentImagePreview.indexOf('const onReady ='), + currentImagePreview.indexOf('if (typeof window ===') + ); + expect(onReady).toContain('onLoadImage()'); + }); +}); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx index 74fb418761f..6600d170839 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx @@ -6,13 +6,15 @@ import type { ProgressImage as ProgressImageType } from 'features/nodes/types/co import { LRUCache } from 'lru-cache'; import { type Atom, atom, computed, map, type MapStore, type WritableAtom } from 'nanostores'; import type { PropsWithChildren } from 'react'; -import { createContext, memo, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; +import { createContext, memo, useCallback, useContext, useEffect, useMemo, useState } from 'react'; import type { S } from 'services/api/types'; import { getEventScope } from 'services/events/eventScope'; import { $socket } from 'services/events/stores'; import { assert } from 'tsafe'; import type { JsonObject } from 'type-fest'; +import { createDeferredClear, getTerminalProgressAction } from './progressImageResolution'; + /** Live progress for a single in-flight session (queue item). Used to tile the viewer when several * sessions run concurrently (multi-GPU). Only items that have produced a preview image are tracked. */ export type ViewerProgressDatum = { @@ -58,11 +60,45 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { )[0]; const $isProgressImageResolving = useState(() => atom(false))[0]; const $isTemporarilyShowingSelectedImage = useState(() => atom(false))[0]; - const shouldClearProgressImageOnLoadRef = useRef(false); + // Owns both the "clear on load" flag and its backstop timer, so no path can reset one and leak + // the other. See createDeferredClear. + const [deferredClear] = useState(() => createDeferredClear()); // We can have race conditions where we receive a progress event for a queue item that has already finished. Easiest // way to handle this is to keep track of finished queue items in a cache and ignore progress events for those. const [finishedQueueItemIds] = useState(() => new LRUCache({ max: 200 })); + // Cancels a pending deferred clear without touching the preview itself. Every path that takes + // responsibility for the preview away from the armed onLoadImage must call this, or the backstop + // outlives the generation that armed it and blanks a later one's live preview. + const disarmDeferredClear = useCallback(() => { + deferredClear.disarm(); + $isProgressImageResolving.set(false); + }, [$isProgressImageResolving, deferredClear]); + + const clearProgressImage = useCallback(() => { + disarmDeferredClear(); + $progressEvent.set(null); + $progressImage.set(null); + }, [disarmDeferredClear, $progressEvent, $progressImage]); + + // Nulling $progressImage tears down the whole overlay, tiles included — $activeProgressData only + // renders while it is set. So when other sessions are still producing previews (multi-GPU), the + // backstop must not clear: the overlay has already stopped being this item's to own. Disarming is + // enough; those sessions clear it via their own terminal events. + const onResolveDeadline = useCallback(() => { + if ($activeProgressData.get().length > 0) { + disarmDeferredClear(); + return; + } + clearProgressImage(); + }, [$activeProgressData, clearProgressImage, disarmDeferredClear]); + + useEffect(() => { + return () => { + deferredClear.disarm(); + }; + }, [deferredClear]); + useEffect(() => { if (!socket) { return; @@ -81,8 +117,10 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { ); return; } - shouldClearProgressImageOnLoadRef.current = false; - $isProgressImageResolving.set(false); + // A new preview supersedes any deferred clear still armed by the previous queue item, whose + // final image may never have loaded. Leaving its backstop running would blank this preview + // mid-generation. + disarmDeferredClear(); $progressEvent.set(data); if (data.image) { $progressImage.set(data.image); @@ -100,7 +138,7 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { return () => { socket.off('invocation_progress', onInvocationProgress); }; - }, [$isProgressImageResolving, $progressData, $progressEvent, $progressImage, finishedQueueItemIds, socket, store]); + }, [$progressData, $progressEvent, $progressImage, disarmDeferredClear, finishedQueueItemIds, socket, store]); useEffect(() => { if (!socket) { @@ -128,39 +166,29 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { // Remove this session's tile from the multi-session preview as soon as it reaches a terminal // state. The single-image "resolve" illusion below is handled separately via onLoadImage. $progressData.setKey(data.item_id, undefined); - // The shared $progressEvent/$progressImage globals may currently hold a DIFFERENT session's - // latest preview (multi-GPU). Only the item that owns them may clear them — otherwise - // canceling item A would blank item B's still-running preview until B's next image event. - const globalProgressEvent = $progressEvent.get(); - if (globalProgressEvent !== null && globalProgressEvent.item_id !== data.item_id) { + + // See getTerminalProgressAction for why each outcome is chosen. 'arm' defers the clear to + // onLoadImage so the viewer can create the illusion of the progress image "resolving" into + // the final image — clearing it here instead would flicker through the previously-selected + // gallery image before the final one appears. + const action = getTerminalProgressAction(data, { + autoSwitch, + globalProgressItemId: $progressEvent.get()?.item_id ?? null, + }); + + if (action === 'ignore') { return; } - // Completed queue items have the progress event cleared by the onLoadImage callback. This allows the viewer to - // create the illusion of the progress image "resolving" into the final image. If we cleared the progress image - // now, there would be a flicker where the progress image disappears before the final image appears, and the - // last-selected gallery image should be shown for a brief moment. - // - // When gallery auto-switch is disabled, we do not need to create this illusion, because we are not going to - // switch to the final image automatically. In this case, we clear the progress image immediately. - // - // We also clear the progress image if the queue item is canceled or failed, as there is no final image to show. - if ( - data.status === 'canceled' || - data.status === 'failed' || - !autoSwitch || - // When the origin is 'canvas' and destination is 'canvas' (without a ':' suffix), that means the - // image is going to be added to the staging area. In this case, we need to clear the progress image else it - // will be stuck on the viewer. - (data.origin === 'canvas' && data.destination !== 'canvas') - ) { - shouldClearProgressImageOnLoadRef.current = false; - $isProgressImageResolving.set(false); - $progressEvent.set(null); - $progressImage.set(null); - } else { - shouldClearProgressImageOnLoadRef.current = true; - $isProgressImageResolving.set(true); + + if (action === 'clear') { + clearProgressImage(); + return; } + + $isProgressImageResolving.set(true); + // onLoadImage is not guaranteed to fire — see PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS. Without + // this deadline the overlay can cover the finished image until the page is reloaded. + deferredClear.arm(onResolveDeadline); } }; @@ -173,23 +201,58 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { $isProgressImageResolving, $progressData, $progressEvent, - $progressImage, autoSwitch, + clearProgressImage, + deferredClear, finishedQueueItemIds, + onResolveDeadline, socket, store, ]); + // The viewer's progress atoms are separate stores from the global ones in services/events/stores, + // which setEventListeners already resets on every socket lifecycle transition. Without the same + // reset here the two diverge: socket.io has no event replay, so a drop spanning the terminal + // queue_item_status_changed loses that event permanently and nothing is left to clear the opaque + // overlay covering the finished image. Backgrounding a tab long enough for the connection to be + // torn down is the common way to hit this. + // + // Clearing on disconnect — not just on reconnect — matches the progress *bars*, which already + // vanish then. If the generation is in fact still running, the next invocation_progress event + // repopulates the preview within a step. + useEffect(() => { + if (!socket) { + return; + } + + const onSocketLifecycleChange = () => { + clearProgressImage(); + // connect_error fires once per reconnection attempt, i.e. roughly once a second while the + // server is down. `set` compares by reference, so an unconditional `set({})` would notify + // every subscriber on every attempt; only replace the map when it actually holds something. + if (Object.keys($progressData.get()).length > 0) { + $progressData.set({}); + } + }; + + socket.on('connect', onSocketLifecycleChange); + socket.on('connect_error', onSocketLifecycleChange); + socket.on('disconnect', onSocketLifecycleChange); + + return () => { + socket.off('connect', onSocketLifecycleChange); + socket.off('connect_error', onSocketLifecycleChange); + socket.off('disconnect', onSocketLifecycleChange); + }; + }, [$progressData, clearProgressImage, socket]); + const onLoadImage = useCallback(() => { - if (!shouldClearProgressImageOnLoadRef.current) { + if (!deferredClear.isArmed()) { return; } - shouldClearProgressImageOnLoadRef.current = false; - $isProgressImageResolving.set(false); - $progressEvent.set(null); - $progressImage.set(null); - }, [$isProgressImageResolving, $progressEvent, $progressImage]); + clearProgressImage(); + }, [clearProgressImage, deferredClear]); const value = useMemo( () => ({ diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts new file mode 100644 index 00000000000..4eec2b44888 --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts @@ -0,0 +1,150 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + createDeferredClear, + getTerminalProgressAction, + PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS, +} from './progressImageResolution'; + +type Event = Parameters[0]; + +const buildEvent = (overrides: Partial = {}): Event => ({ + item_id: 1, + status: 'completed', + origin: null, + destination: null, + ...overrides, +}); + +const OWNED = { autoSwitch: true, globalProgressItemId: 1 }; + +describe('getTerminalProgressAction', () => { + it('defers the clear to the image load for a completed item when auto-switching', () => { + expect(getTerminalProgressAction(buildEvent(), OWNED)).toBe('arm'); + }); + + it('defers the clear when no item owns the shared progress atoms yet', () => { + expect(getTerminalProgressAction(buildEvent(), { autoSwitch: true, globalProgressItemId: null })).toBe('arm'); + }); + + it.each(['canceled', 'failed'] as const)('clears immediately for a %s item, as nothing will load', (status) => { + expect(getTerminalProgressAction(buildEvent({ status }), OWNED)).toBe('clear'); + }); + + it('clears immediately when auto-switch is off, since the viewer will not show the final image', () => { + expect(getTerminalProgressAction(buildEvent(), { ...OWNED, autoSwitch: false })).toBe('clear'); + }); + + it('clears immediately for a canvas item bound for the staging area', () => { + const event = buildEvent({ origin: 'canvas', destination: 'canvas_session_1' }); + expect(getTerminalProgressAction(event, OWNED)).toBe('clear'); + }); + + it('still defers for a canvas item that stays in the viewer', () => { + const event = buildEvent({ origin: 'canvas', destination: 'canvas' }); + expect(getTerminalProgressAction(event, OWNED)).toBe('arm'); + }); + + it('ignores an item that does not own the shared progress atoms', () => { + // Multi-GPU: canceling item 2 must not blank item 1's still-running preview. + const event = buildEvent({ item_id: 2, status: 'canceled' }); + expect(getTerminalProgressAction(event, { autoSwitch: true, globalProgressItemId: 1 })).toBe('ignore'); + }); + + it('ignores a non-owning item even when it completed successfully', () => { + const event = buildEvent({ item_id: 2 }); + expect(getTerminalProgressAction(event, { autoSwitch: true, globalProgressItemId: 1 })).toBe('ignore'); + }); + + it('uses a backstop long enough not to fire on the normal thumbnail-gated path', () => { + expect(PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS).toBeGreaterThanOrEqual(5_000); + }); +}); + +describe('createDeferredClear', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('runs the deadline callback when nothing disarms it', () => { + const onDeadline = vi.fn(); + const deferred = createDeferredClear(1_000); + + deferred.arm(onDeadline); + expect(deferred.isArmed()).toBe(true); + + vi.advanceTimersByTime(1_000); + + expect(onDeadline).toHaveBeenCalledOnce(); + expect(deferred.isArmed()).toBe(false); + }); + + it('never runs the deadline callback after a disarm', () => { + // The regression: item N arms, its final image never loads, item N+1 emits progress (which + // disarms). N's deadline must not fire later and blank N+1's live preview. + const onDeadline = vi.fn(); + const deferred = createDeferredClear(1_000); + + deferred.arm(onDeadline); + deferred.disarm(); + expect(deferred.isArmed()).toBe(false); + + vi.advanceTimersByTime(60_000); + + expect(onDeadline).not.toHaveBeenCalled(); + }); + + it('supersedes the previous deadline when re-armed rather than stacking', () => { + const first = vi.fn(); + const second = vi.fn(); + const deferred = createDeferredClear(1_000); + + deferred.arm(first); + vi.advanceTimersByTime(900); + deferred.arm(second); + + // The first deadline's original moment passes with nothing pending for it. + vi.advanceTimersByTime(100); + expect(first).not.toHaveBeenCalled(); + expect(second).not.toHaveBeenCalled(); + + // The re-arm restarted the clock, so the second fires a full interval after it was armed. + vi.advanceTimersByTime(900); + expect(first).not.toHaveBeenCalled(); + expect(second).toHaveBeenCalledOnce(); + }); + + it('fires at most once per arm', () => { + const onDeadline = vi.fn(); + const deferred = createDeferredClear(1_000); + + deferred.arm(onDeadline); + vi.advanceTimersByTime(10_000); + + expect(onDeadline).toHaveBeenCalledOnce(); + }); + + it('tolerates disarming when nothing is armed', () => { + const deferred = createDeferredClear(1_000); + + expect(() => { + deferred.disarm(); + deferred.disarm(); + }).not.toThrow(); + expect(deferred.isArmed()).toBe(false); + }); + + it('reports not-armed once the deadline has fired, so a late load is a no-op', () => { + // onLoadImage is gated on isArmed(); a load arriving after the backstop already cleared must + // not clear a preview that a newer generation has since put up. + const deferred = createDeferredClear(1_000); + deferred.arm(vi.fn()); + vi.advanceTimersByTime(1_000); + + expect(deferred.isArmed()).toBe(false); + }); +}); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts new file mode 100644 index 00000000000..7c39bae95ee --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts @@ -0,0 +1,126 @@ +import type { S } from 'services/api/types'; + +/** + * Backstop for the deferred progress-image clear. + * + * A completed queue item hands responsibility for clearing the viewer's progress preview to the + * final image's load callback, so the preview appears to resolve into the finished image instead of + * flickering through the previously-selected one. That callback is not guaranteed to fire: + * - the image request can fail, which Chakra reports as `onError`, not `onLoad`; + * - the finished image can already be the one on screen, and Chakra's `useImage` only re-runs when + * `src` changes; + * - the load can beat the terminal queue event, leaving nothing to trigger the clear afterwards; + * - the item's outputs can all be intermediate, so no selection change ever happens. + * + * Any of those wedges the opaque overlay over the finished image until the page is reloaded, so the + * armed state needs a deadline. + * + * Deliberately long. This is a backstop against a state that would otherwise be permanent, not a + * latency target: the reveal is gated on the thumbnail, so the normal path resolves in well under a + * second. Firing early is its own regression — it replaces the preview with the previously-selected + * gallery image, or with nothing at all — so the deadline sits well beyond how long a ~20KB + * thumbnail can plausibly take, even on a bad connection. + */ +export const PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS = 30_000; + +type TerminalProgressAction = + /** Clear the progress preview now. */ + | 'clear' + /** Defer the clear until the final image loads, or until the backstop above fires. */ + | 'arm' + /** This event does not own the shared progress preview — leave it alone. */ + | 'ignore'; + +/** The fields of a terminal `queue_item_status_changed` event the decision depends on. */ +type TerminalQueueItemEvent = Pick; + +type TerminalProgressActionOptions = { + /** Whether the gallery auto-switches to the finished image. */ + autoSwitch: boolean; + /** The item id currently owning the shared progress atoms, or null when they are unset. */ + globalProgressItemId: number | null; +}; + +/** + * Decides what a terminal `queue_item_status_changed` event should do to the viewer's progress + * preview. Pure, so the branchy policy is testable without a socket or a React tree — the caller + * owns the side effects. + */ +export const getTerminalProgressAction = ( + data: TerminalQueueItemEvent, + { autoSwitch, globalProgressItemId }: TerminalProgressActionOptions +): TerminalProgressAction => { + // The shared progress atoms may currently hold a DIFFERENT session's latest preview (multi-GPU). + // Only the item that owns them may clear them — otherwise canceling item A would blank item B's + // still-running preview until B's next image event. + if (globalProgressItemId !== null && globalProgressItemId !== data.item_id) { + return 'ignore'; + } + + // Nothing is going to load in place of the preview, so there is no resolve illusion to create. + if (data.status === 'canceled' || data.status === 'failed') { + return 'clear'; + } + + // Auto-switch off means the viewer is not going to show the finished image at all. + if (!autoSwitch) { + return 'clear'; + } + + // Origin 'canvas' with a destination that is not 'canvas' (i.e. without a ':' suffix) + // means the image is bound for the staging area rather than this viewer, so nothing will ever + // load here to clear the preview. + if (data.origin === 'canvas' && data.destination !== 'canvas') { + return 'clear'; + } + + return 'arm'; +}; + +type DeferredClear = { + /** + * Arms the deferred clear, replacing any deadline already pending. `onDeadline` runs only if + * nothing disarms first. + */ + arm: (onDeadline: () => void) => void; + /** Cancels a pending deadline. Safe to call when nothing is armed. */ + disarm: () => void; + /** True between `arm` and the next `disarm`, or until the deadline fires. */ + isArmed: () => boolean; +}; + +/** + * Owns the armed flag and its backstop timer as one unit. + * + * Keeping them together is the point: when they were two independent pieces of state, a path that + * reset the flag but forgot the timer left the deadline running past the generation that armed it, + * so it fired later and blanked a subsequent generation's live preview. Arming always supersedes + * the previous deadline rather than stacking, and disarming always cancels it. + * + * Uses bare `setTimeout`/`clearTimeout` rather than the `window` members so this stays usable — and + * testable with fake timers — outside a DOM. + */ +export const createDeferredClear = (timeoutMs: number = PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS): DeferredClear => { + let armed = false; + let timeoutId: ReturnType | null = null; + + const disarm = () => { + armed = false; + if (timeoutId !== null) { + clearTimeout(timeoutId); + timeoutId = null; + } + }; + + const arm = (onDeadline: () => void) => { + disarm(); + armed = true; + timeoutId = setTimeout(() => { + timeoutId = null; + armed = false; + onDeadline(); + }, timeoutMs); + }; + + return { arm, disarm, isArmed: () => armed }; +}; From d86b6f60964d3c6610f90e88f1423cebfce45e36 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Mon, 3 Aug 2026 11:51:39 -0400 Subject: [PATCH 2/8] fix(ui): stop auto-switch flashing the previous image over the next preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting a new generation soon after the previous one finishes made the viewer flicker: the new previews would appear, then the previous generation's finished image would cover them for two seconds, then the previews resumed. Waiting between generations avoided it. The flash is the "reveal selected image" feature (#9217), which briefly hides the progress overlay so a mid-generation gallery click is visible. Its only guard against the auto-switch handoff was $isProgressImageResolving — a timing guard, and the timing loses: the auto-switch selection is dispatched only after onInvocationComplete's async DTO fetch, then waits for the thumbnail preload, and the next generation's first invocation_progress event slots into that window and resets the flag. By the time the handoff reaches the viewer it is indistinguishable from a user click, so the reveal fires over the live preview. Distinguish them by identity instead of timing: auto-switch records the image name in a small registry at dispatch, and the reveal effect consumes it on the selection's first render. Consumption happens on every rendered-image change, not only when the reveal conditions hold, because in the common (unraced) case the image renders with no progress showing and a leftover entry would suppress a genuine user selection of the same image later. Entries also expire after 30 seconds. Recording is unconditional but consumption requires the image to actually render, so a superseded auto-switch (two completions within one thumbnail-fetch window — routine with parallel multi-GPU sessions), a viewer unmounted by comparison mode, or a duplicate invocation_complete event would otherwise leave an immortal entry whose only future effect is to swallow a genuine click on that image — the very dead-click the reveal exists to prevent. The TTL is generous for the dispatch-to-render handoff it protects; expiring early merely readmits the 2-second flash on a very slow connection, which is the milder failure. The suppression branch still lowers $isTemporarilyShowingSelectedImage — the effect has already cancelled any running reveal's timer by that point, so returning with the atom raised would wedge the reveal on. Co-Authored-By: Claude Fable 5 --- .../ImageViewer/CurrentImagePreview.tsx | 19 +++++ .../gallery/store/autoSwitchedImages.test.ts | 65 +++++++++++++++++ .../gallery/store/autoSwitchedImages.ts | 70 +++++++++++++++++++ .../services/events/onInvocationComplete.tsx | 7 ++ 4 files changed, 161 insertions(+) create mode 100644 invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts create mode 100644 invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx index 06550899a2d..cc9d03e97b7 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -7,6 +7,7 @@ import { DndImage } from 'features/dnd/DndImage'; import ImageMetadataViewer from 'features/gallery/components/ImageMetadataViewer/ImageMetadataViewer'; import NextPrevItemButtons from 'features/gallery/components/NextPrevItemButtons'; import { useNextPrevItemNavigation } from 'features/gallery/components/useNextPrevItemNavigation'; +import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; import { useRegisteredHotkeys } from 'features/system/components/HotkeysModal/useHotkeyData'; import { navigationApi } from 'features/ui/layouts/navigation-api'; @@ -115,6 +116,14 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu const previousRenderedImageName = previousRenderedImageNameRef.current; previousRenderedImageNameRef.current = renderedImageName; + // Consume on every change of the rendered image, not only when the reveal conditions below + // hold — in the common case the auto-switched image renders with no progress showing, and an + // entry left behind would suppress a genuine user selection of the same image later. + const wasAutoSwitchedTo = + renderedImageName !== null && + renderedImageName !== previousRenderedImageName && + autoSwitchedImages.consume(renderedImageName); + window.clearTimeout(selectedImageRevealTimeoutId.current); if ( @@ -132,6 +141,16 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu return; } + // The reveal exists to make a mid-generation *user* selection visible. An auto-switch to a + // just-finished image can land here late — after the next generation's first progress event + // has already reset $isProgressImageResolving — and must not flash the previous result over + // the live preview. The set(false) is required: the clearTimeout above already cancelled any + // running reveal's timer, so returning with the atom still true would wedge the reveal on. + if (wasAutoSwitchedTo) { + $isTemporarilyShowingSelectedImage.set(false); + return; + } + $isTemporarilyShowingSelectedImage.set(true); selectedImageRevealTimeoutId.current = window.setTimeout(() => { $isTemporarilyShowingSelectedImage.set(false); diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts new file mode 100644 index 00000000000..0f3ecb4040d --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from 'vitest'; + +import { createAutoSwitchedImageRegistry } from './autoSwitchedImages'; + +describe('createAutoSwitchedImageRegistry', () => { + it('consumes a recorded name exactly once', () => { + const registry = createAutoSwitchedImageRegistry(); + registry.record('a.png'); + expect(registry.consume('a.png')).toBe(true); + expect(registry.consume('a.png')).toBe(false); + }); + + it('returns false for a name that was never recorded', () => { + const registry = createAutoSwitchedImageRegistry(); + expect(registry.consume('a.png')).toBe(false); + }); + + it('tracks multiple pending names independently', () => { + const registry = createAutoSwitchedImageRegistry(); + registry.record('a.png'); + registry.record('b.png'); + expect(registry.consume('b.png')).toBe(true); + expect(registry.consume('a.png')).toBe(true); + expect(registry.consume('b.png')).toBe(false); + }); + + it('evicts the oldest entry beyond the bound', () => { + const registry = createAutoSwitchedImageRegistry(); + for (let i = 0; i < 9; i++) { + registry.record(`image-${i}.png`); + } + // 9 recorded, bound is 8 — the oldest is gone, the rest remain. + expect(registry.consume('image-0.png')).toBe(false); + for (let i = 1; i < 9; i++) { + expect(registry.consume(`image-${i}.png`)).toBe(true); + } + }); + + it('expires entries after the TTL', () => { + let t = 0; + const registry = createAutoSwitchedImageRegistry(() => t); + registry.record('a.png'); + t = 30_001; + expect(registry.consume('a.png')).toBe(false); + }); + + it('keeps entries up to the TTL boundary', () => { + let t = 0; + const registry = createAutoSwitchedImageRegistry(() => t); + registry.record('a.png'); + t = 30_000; + expect(registry.consume('a.png')).toBe(true); + }); + + it('prunes expired entries without touching live ones', () => { + let t = 0; + const registry = createAutoSwitchedImageRegistry(() => t); + registry.record('old.png'); + t = 20_000; + registry.record('new.png'); + t = 40_000; + expect(registry.consume('old.png')).toBe(false); + expect(registry.consume('new.png')).toBe(true); + }); +}); diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts new file mode 100644 index 00000000000..335d5434fac --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts @@ -0,0 +1,70 @@ +/** + * Names of images the gallery auto-switched to, pending their first render in the viewer. + * + * The viewer briefly reveals a newly selected image over the progress overlay so that a + * mid-generation gallery click is not invisible (see the reveal effect in CurrentImagePreview). + * Auto-switch selections must not trigger that reveal — but they land asynchronously: the switch is + * dispatched only after onInvocationComplete's DTO fetch resolves, and the viewer renders it only + * after the thumbnail preload settles. When the next generation is started quickly, its first + * invocation_progress event slots into that window and resets $isProgressImageResolving, so by the + * time the auto-switch selection reaches the viewer it is indistinguishable from a user click and + * the reveal flashes the previous result over the live preview for 2 seconds. + * + * Recording the image name at dispatch and consuming it on the selection's first render + * distinguishes the two without depending on event timing. + */ +export type AutoSwitchedImageRegistry = { + /** Records that the gallery is auto-switching to this image. */ + record: (imageName: string) => void; + /** + * Returns whether this image was recently auto-switched to, removing the entry. Call exactly + * once per rendered-image change — an entry left behind would suppress a genuine user selection + * of the same image later. + */ + consume: (imageName: string) => boolean; +}; + +// A selection can be superseded before it ever renders (rapid back-to-back completions), leaving +// its entry unconsumed. The bound keeps those leftovers from accumulating; a dropped entry's worst +// case is one spurious 2-second reveal. +const MAX_PENDING = 8; + +// An entry is only meaningful for the handoff window between the auto-switch dispatch and the +// image's first render (redux propagation plus the thumbnail preload). An entry that outlives that +// window is an orphan — its selection was superseded before rendering, the viewer was unmounted +// (comparison mode), or a duplicate completion event re-recorded an already-rendered image — and +// consuming an orphan later would swallow a genuine user click on that image, the very dead-click +// the reveal exists to prevent. Generous enough for a slow thumbnail fetch (same reasoning as +// PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS); expiring early merely readmits the 2-second flash on a very +// slow connection, which is the milder failure. +const TTL_MS = 30_000; + +export const createAutoSwitchedImageRegistry = (now: () => number = Date.now): AutoSwitchedImageRegistry => { + let pending: { imageName: string; recordedAt: number }[] = []; + + const prune = () => { + const cutoff = now() - TTL_MS; + pending = pending.filter((entry) => entry.recordedAt >= cutoff); + }; + + return { + record: (imageName) => { + prune(); + pending.push({ imageName, recordedAt: now() }); + if (pending.length > MAX_PENDING) { + pending.shift(); + } + }, + consume: (imageName) => { + prune(); + const index = pending.findIndex((entry) => entry.imageName === imageName); + if (index === -1) { + return false; + } + pending.splice(index, 1); + return true; + }, + }; +}; + +export const autoSwitchedImages = createAutoSwitchedImageRegistry(); diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx index 5318043749d..9bbb732b09e 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx @@ -1,6 +1,7 @@ import { logger } from 'app/logging/logger'; import type { AppDispatch, AppGetState } from 'app/store/store'; import { canvasWorkflowIntegrationProcessingCompleted } from 'features/controlLayers/store/canvasWorkflowIntegrationSlice'; +import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; import { selectAutoSwitch, selectGalleryView, @@ -196,6 +197,12 @@ export const buildOnInvocationComplete = ( const { image_name } = lastImageDTO; const board_id = lastImageDTO.board_id ?? 'none'; + // Both branches below auto-switch the selection to this image. Record that so the viewer's + // reveal effect can tell the handoff apart from a user's gallery click — this dispatch happens + // after an async DTO fetch, so it can land after the next generation's first progress event has + // already reset $isProgressImageResolving, and timing alone cannot distinguish the two. + autoSwitchedImages.record(image_name); + // With optimistic updates, we can immediately switch to the new image const selectedBoardId = selectSelectedBoardId(getState()); From 978092a6843d9b78b9d5bb592630e6a1e2a2a88b Mon Sep 17 00:00:00 2001 From: JPPhoto Date: Sat, 8 Aug 2026 12:16:53 -0500 Subject: [PATCH 3/8] chore(ui): resolve knip warnings --- .../web/src/features/gallery/store/autoSwitchedImages.ts | 2 +- invokeai/frontend/web/src/services/api/endpoints/videos.ts | 5 ++--- invokeai/frontend/web/src/services/api/types.ts | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts index 335d5434fac..ff51745e287 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts @@ -13,7 +13,7 @@ * Recording the image name at dispatch and consuming it on the selection's first render * distinguishes the two without depending on event timing. */ -export type AutoSwitchedImageRegistry = { +type AutoSwitchedImageRegistry = { /** Records that the gallery is auto-switching to this image. */ record: (imageName: string) => void; /** diff --git a/invokeai/frontend/web/src/services/api/endpoints/videos.ts b/invokeai/frontend/web/src/services/api/endpoints/videos.ts index 49043d763a0..3f7327ca49c 100644 --- a/invokeai/frontend/web/src/services/api/endpoints/videos.ts +++ b/invokeai/frontend/web/src/services/api/endpoints/videos.ts @@ -352,14 +352,13 @@ export const { useDeleteUncategorizedVideosMutation, } = videosApi; +export const { useGetVideoMetadataQuery, useGetVideoWorkflowQuery, useLazyGetVideoWorkflowQuery } = videosApi; + /** @knipignore Reserved for follow-up phases (bulk delete / intermediate toggle / video-only views). * useDeleteVideoMutation is here because the only call site uses videosApi.endpoints.deleteVideo.initiate * via the delete-video modal, but a future bulk/multi-select flow may want the React hook form. */ export const { useListVideosQuery, - useGetVideoMetadataQuery, - useGetVideoWorkflowQuery, - useLazyGetVideoWorkflowQuery, useGetVideoNamesQuery, useDeleteVideoMutation, useDeleteVideosMutation, diff --git a/invokeai/frontend/web/src/services/api/types.ts b/invokeai/frontend/web/src/services/api/types.ts index 344633fe03b..de754a103d9 100644 --- a/invokeai/frontend/web/src/services/api/types.ts +++ b/invokeai/frontend/web/src/services/api/types.ts @@ -733,7 +733,6 @@ export type UploadVideoArg = { export type GalleryItem = S['GalleryItem']; /** @knipignore Consumed by gallery wiring in Phase 4. */ export type GalleryItemKind = S['GalleryItemKind']; -/** @knipignore Consumed by gallery wiring in Phase 4. */ export type GalleryItemRef = S['GalleryItemRef']; /** @knipignore Consumed by gallery wiring in Phase 4. */ export type GalleryItemNamesResult = S['GalleryItemNamesResult']; From 450897d572fc80c7bdc851848ccb9600d0c5a80e Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 19:27:16 -0400 Subject: [PATCH 4/8] =?UTF-8?q?fix(ui):=20address=20review=20=E2=80=94=20d?= =?UTF-8?q?eadline=20ownership=20handoff=20+=20duplicate-completion=20gall?= =?UTF-8?q?ery=20work?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from JPPhoto's review: 1. When the resolve deadline fired while other sessions were still active (multi-GPU), it only disarmed, leaving the shared progress atoms owned by the finished item. The surviving sessions' terminal events then saw a foreign owner and ignored them, stranding the opaque overlay on a stale preview after the last session ended. The deadline now promotes the most recently active session into the shared atoms, so its own terminal event clears or re-arms them normally. 2. Duplicate invocation_complete deliveries re-ran the gallery work, which double-counted optimistic board totals and re-recorded the auto-switch marker after it had been consumed — suppressing a later genuine gallery click on that image. The handler now tracks processed invocations itself (the shared dedupe map can't be used: the workflow coordinator pre-marks first-delivery events for non-active workflow items) and returns before any gallery work on a duplicate. The auto-switch registry additionally settles on every rendered-image change: a match drops all older entries, a miss clears the registry, so no stale entry survives past the next render to swallow a genuine click. Co-Authored-By: Claude Fable 5 --- .../ImageViewer/CurrentImagePreview.tsx | 4 +- .../components/ImageViewer/context.test.ts | 15 +++++ .../components/ImageViewer/context.tsx | 17 ++++-- .../progressImageResolution.test.ts | 31 ++++++++++ .../ImageViewer/progressImageResolution.ts | 35 +++++++++++ .../gallery/store/autoSwitchedImages.test.ts | 54 ++++++++++++++-- .../gallery/store/autoSwitchedImages.ts | 36 +++++++---- .../events/onInvocationComplete.test.ts | 61 +++++++++++++++++++ .../services/events/onInvocationComplete.tsx | 23 +++++++ 9 files changed, 250 insertions(+), 26 deletions(-) diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx index cc9d03e97b7..678e039a540 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -117,8 +117,8 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu previousRenderedImageNameRef.current = renderedImageName; // Consume on every change of the rendered image, not only when the reveal conditions below - // hold — in the common case the auto-switched image renders with no progress showing, and an - // entry left behind would suppress a genuine user selection of the same image later. + // hold — each render settles the registry (see autoSwitchedImages.consume), so no stale entry + // survives to suppress a genuine user selection of the same image later. const wasAutoSwitchedTo = renderedImageName !== null && renderedImageName !== previousRenderedImageName && diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts index c2a7dc8b597..2f4e964e106 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.test.ts @@ -44,6 +44,21 @@ describe('ImageViewer progress image wiring', () => { expect(progressHandler).toContain('disarmDeferredClear()'); }); + it('promotes a still-active session at the resolve deadline instead of merely disarming', () => { + // Disarm-only leaves the shared atoms owned by the finished item, so the surviving sessions' + // terminal events 'ignore' them as foreign and the overlay strands on a stale preview after + // the last session ends. The promotion policy itself is tested in progressImageResolution. + const deadlineHandler = context.slice( + context.indexOf('const onResolveDeadline ='), + context.indexOf('useEffect(() => {') + ); + expect(deadlineHandler).toContain('pickPromotionCandidate($activeProgressData.get())'); + expect(deadlineHandler).toContain('$progressEvent.set(candidate.progressEvent)'); + expect(deadlineHandler).toContain('$progressImage.set(candidate.progressImage)'); + expect(deadlineHandler).toContain('disarmDeferredClear()'); + expect(deadlineHandler).toContain('clearProgressImage()'); + }); + it('gates the viewer reveal on the thumbnail rather than the full-resolution image', () => { // Gating on `/full` holds a stale latent preview on screen for the whole multi-megabyte // download on a slow connection. diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx index 6600d170839..50768a807be 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx @@ -13,7 +13,7 @@ import { $socket } from 'services/events/stores'; import { assert } from 'tsafe'; import type { JsonObject } from 'type-fest'; -import { createDeferredClear, getTerminalProgressAction } from './progressImageResolution'; +import { createDeferredClear, getTerminalProgressAction, pickPromotionCandidate } from './progressImageResolution'; /** Live progress for a single in-flight session (queue item). Used to tile the viewer when several * sessions run concurrently (multi-GPU). Only items that have produced a preview image are tracked. */ @@ -83,15 +83,22 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { // Nulling $progressImage tears down the whole overlay, tiles included — $activeProgressData only // renders while it is set. So when other sessions are still producing previews (multi-GPU), the - // backstop must not clear: the overlay has already stopped being this item's to own. Disarming is - // enough; those sessions clear it via their own terminal events. + // backstop must not clear: the overlay has already stopped being this item's to own. Merely + // disarming is not enough either — that leaves the shared atoms owned by the finished item, and + // the surviving sessions' terminal events would then 'ignore' them as foreign (see + // getTerminalProgressAction), stranding the overlay on a stale preview after the last session + // ends. Hand the atoms to the most recently active session instead; its own terminal event then + // clears or re-arms them normally. const onResolveDeadline = useCallback(() => { - if ($activeProgressData.get().length > 0) { + const candidate = pickPromotionCandidate($activeProgressData.get()); + if (candidate) { disarmDeferredClear(); + $progressEvent.set(candidate.progressEvent); + $progressImage.set(candidate.progressImage); return; } clearProgressImage(); - }, [$activeProgressData, clearProgressImage, disarmDeferredClear]); + }, [$activeProgressData, $progressEvent, $progressImage, clearProgressImage, disarmDeferredClear]); useEffect(() => { return () => { diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts index 4eec2b44888..74e92c81dc6 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.test.ts @@ -3,6 +3,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createDeferredClear, getTerminalProgressAction, + pickPromotionCandidate, PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS, } from './progressImageResolution'; @@ -61,6 +62,36 @@ describe('getTerminalProgressAction', () => { }); }); +describe('pickPromotionCandidate', () => { + const datum = (itemId: number, timestamp: number) => ({ itemId, progressEvent: { timestamp } }); + + it('returns null when no session is active, so the deadline clears', () => { + expect(pickPromotionCandidate([])).toBeNull(); + }); + + it('picks the session with the newest progress event', () => { + expect(pickPromotionCandidate([datum(1, 300), datum(2, 100), datum(3, 200)])).toEqual(datum(1, 300)); + }); + + it('breaks timestamp ties toward the later-enqueued item', () => { + expect(pickPromotionCandidate([datum(1, 100), datum(2, 100)])).toEqual(datum(2, 100)); + }); + + it('transfers ownership so the promoted item is not ignored at its own terminal event', () => { + // Regression: progress B, progress A (A owns the shared atoms), complete A, deadline fires with + // B still active. Merely disarming left the owner as A, so B's terminal event returned 'ignore' + // and the overlay stuck with A's stale preview forever. Promoting B makes its terminal event + // actionable again. + const promoted = pickPromotionCandidate([datum(2, 100)]); + expect(promoted).not.toBeNull(); + const action = getTerminalProgressAction(buildEvent({ item_id: 2 }), { + autoSwitch: true, + globalProgressItemId: promoted?.itemId ?? null, + }); + expect(action).toBe('arm'); + }); +}); + describe('createDeferredClear', () => { beforeEach(() => { vi.useFakeTimers(); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts index 7c39bae95ee..7b5b1b58ebb 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/progressImageResolution.ts @@ -77,6 +77,41 @@ export const getTerminalProgressAction = ( return 'arm'; }; +/** The fields of a per-session progress datum the promotion choice depends on. */ +type PromotionCandidate = { + itemId: number; + progressEvent: { timestamp: number }; +}; + +/** + * Picks which still-active session should inherit the shared progress atoms when the resolve + * deadline fires while other sessions are running (multi-GPU). + * + * Merely disarming at the deadline leaves the atoms owned by the finished item. The surviving + * sessions' terminal events then see a foreign owner and 'ignore' (see getTerminalProgressAction), + * so nothing is left to clear the overlay once the last of them ends — it sticks with the finished + * item's stale preview until the page is reloaded. Promoting an active session transfers ownership + * to an item whose own terminal event will clear or re-arm normally, and replaces the stale preview + * with that session's latest known one. + * + * The candidate is the session with the newest progress event — what the shared atoms would hold + * had it emitted last — with ties broken toward the later-enqueued item. Returns null when no + * session is active, in which case the deadline should clear instead. + */ +export const pickPromotionCandidate = (activeData: readonly T[]): T | null => { + let candidate: T | null = null; + for (const datum of activeData) { + if ( + candidate === null || + datum.progressEvent.timestamp > candidate.progressEvent.timestamp || + (datum.progressEvent.timestamp === candidate.progressEvent.timestamp && datum.itemId > candidate.itemId) + ) { + candidate = datum; + } + } + return candidate; +}; + type DeferredClear = { /** * Arms the deferred clear, replacing any deadline already pending. `onDeadline` runs only if diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts index 0f3ecb4040d..da00b54bce3 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts @@ -15,25 +15,67 @@ describe('createAutoSwitchedImageRegistry', () => { expect(registry.consume('a.png')).toBe(false); }); - it('tracks multiple pending names independently', () => { + it('consumes entries that render in record order', () => { const registry = createAutoSwitchedImageRegistry(); registry.record('a.png'); registry.record('b.png'); - expect(registry.consume('b.png')).toBe(true); expect(registry.consume('a.png')).toBe(true); + expect(registry.consume('b.png')).toBe(true); expect(registry.consume('b.png')).toBe(false); }); + it('drops entries recorded before the consumed one — their selections were superseded', () => { + // Two completions within one thumbnail-fetch window: only b renders. If a's entry survived, + // it would suppress a genuine user click on a during the next generation. + const registry = createAutoSwitchedImageRegistry(); + registry.record('a.png'); + registry.record('b.png'); + expect(registry.consume('b.png')).toBe(true); + expect(registry.consume('a.png')).toBe(false); + }); + + it('clears all pending entries when an unrecorded image renders', () => { + // A rendered image that was never recorded is user activity: any pending selections have been + // superseded and will never first-render, so their entries must not linger to swallow a later + // genuine click. + const registry = createAutoSwitchedImageRegistry(); + registry.record('a.png'); + expect(registry.consume('user-click.png')).toBe(false); + registry.record('b.png'); + expect(registry.consume('a.png')).toBe(false); + // The miss above also settled b.png away. + expect(registry.consume('b.png')).toBe(false); + }); + + it('lets a genuine re-selection reveal after a stale entry is settled by a later render', () => { + // JPPhoto's review sequence: a is recorded again after it already rendered (stale entry), then + // the user selects b -> a during the next generation. The b render must settle the stale entry + // so the click on a reveals. + const registry = createAutoSwitchedImageRegistry(); + registry.record('a.png'); + expect(registry.consume('a.png')).toBe(true); // auto-switch renders a + registry.record('a.png'); // stale re-record + expect(registry.consume('b.png')).toBe(false); // user clicks b — settles the registry + expect(registry.consume('a.png')).toBe(false); // user clicks a — reveal must fire + }); + it('evicts the oldest entry beyond the bound', () => { const registry = createAutoSwitchedImageRegistry(); for (let i = 0; i < 9; i++) { registry.record(`image-${i}.png`); } - // 9 recorded, bound is 8 — the oldest is gone, the rest remain. + // 9 recorded, bound is 8 — the oldest is gone. expect(registry.consume('image-0.png')).toBe(false); - for (let i = 1; i < 9; i++) { - expect(registry.consume(`image-${i}.png`)).toBe(true); + }); + + it('retains the newest entries under the bound', () => { + const registry = createAutoSwitchedImageRegistry(); + for (let i = 0; i < 9; i++) { + registry.record(`image-${i}.png`); } + // image-0 was evicted by the bound, so image-1 is the oldest survivor. + expect(registry.consume('image-1.png')).toBe(true); + expect(registry.consume('image-8.png')).toBe(true); }); it('expires entries after the TTL', () => { @@ -59,7 +101,7 @@ describe('createAutoSwitchedImageRegistry', () => { t = 20_000; registry.record('new.png'); t = 40_000; - expect(registry.consume('old.png')).toBe(false); + // old.png has expired; new.png is still live and consumable. expect(registry.consume('new.png')).toBe(true); }); }); diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts index ff51745e287..22e6b124292 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts @@ -17,24 +17,33 @@ type AutoSwitchedImageRegistry = { /** Records that the gallery is auto-switching to this image. */ record: (imageName: string) => void; /** - * Returns whether this image was recently auto-switched to, removing the entry. Call exactly - * once per rendered-image change — an entry left behind would suppress a genuine user selection - * of the same image later. + * Returns whether this image was recently auto-switched to. Call exactly once per rendered-image + * change: every call settles the registry, because each render proves what became of the pending + * selections. A match removes the entry and every entry recorded before it — those selections + * were superseded and will never first-render (e.g. two completions within one thumbnail-fetch + * window; only the last one renders). A miss means an image that was never recorded rendered, + * i.e. user activity superseded every pending selection, so the registry is cleared entirely. + * Either way, no entry survives past the next rendered-image change to swallow a genuine user + * click later — the very dead-click the reveal exists to prevent. + * + * The miss-clear is deliberately over-eager: a user click made *before* an entry was recorded + * can render *after* it (its preload was already in flight), wiping that live entry and + * readmitting one 2-second flash. That interleave is narrow, and trading it for stale entries + * that swallow clicks would be backwards — the flash is the milder failure. */ consume: (imageName: string) => boolean; }; -// A selection can be superseded before it ever renders (rapid back-to-back completions), leaving -// its entry unconsumed. The bound keeps those leftovers from accumulating; a dropped entry's worst -// case is one spurious 2-second reveal. +// consume settles the registry on every rendered-image change, so entries can only accumulate +// while nothing renders at all — the viewer unmounted by comparison mode while generations keep +// completing. The bound caps memory there; a dropped entry's worst case is one spurious 2-second +// reveal. const MAX_PENDING = 8; -// An entry is only meaningful for the handoff window between the auto-switch dispatch and the -// image's first render (redux propagation plus the thumbnail preload). An entry that outlives that -// window is an orphan — its selection was superseded before rendering, the viewer was unmounted -// (comparison mode), or a duplicate completion event re-recorded an already-rendered image — and -// consuming an orphan later would swallow a genuine user click on that image, the very dead-click -// the reveal exists to prevent. Generous enough for a slow thumbnail fetch (same reasoning as +// Same no-renders window as above: an entry is only meaningful between the auto-switch dispatch +// and the image's first render, and with the viewer unmounted that render may never come. Without +// the TTL, remounting the viewer within reach of such an orphan and clicking its image would +// suppress the reveal. Generous enough for a slow thumbnail fetch (same reasoning as // PROGRESS_IMAGE_RESOLVE_TIMEOUT_MS); expiring early merely readmits the 2-second flash on a very // slow connection, which is the milder failure. const TTL_MS = 30_000; @@ -59,9 +68,10 @@ export const createAutoSwitchedImageRegistry = (now: () => number = Date.now): A prune(); const index = pending.findIndex((entry) => entry.imageName === imageName); if (index === -1) { + pending = []; return false; } - pending.splice(index, 1); + pending = pending.slice(index + 1); return true; }, }; diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts index 0a811360ae0..66467d06559 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts @@ -288,6 +288,67 @@ describe('onInvocationComplete polymorphic gallery cache', () => { expect(galleryInvalidation?.payload).toContainEqual({ type: 'BoardVideosTotal', id: 'board-123' }); expect(galleryInvalidation?.payload).toContain('VirtualBoards'); }); + + it('processes each completion event exactly once — a duplicate delivery does no gallery work', async () => { + // Re-running the gallery handling on a duplicate double-counts the optimistic board totals and + // re-records the auto-switch marker after it was consumed, which suppresses a later genuine + // gallery click on that image. + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await handler(buildImageCompleteEvent()); + const dispatchCountAfterFirst = dispatch.mock.calls.length; + expect(getImageDTOSafe).toHaveBeenCalledTimes(1); + + await handler(buildImageCompleteEvent()); + expect(getImageDTOSafe).toHaveBeenCalledTimes(1); + expect(dispatch.mock.calls.length).toBe(dispatchCountAfterFirst); + }); + + it('rejects a duplicate that arrives while the first delivery is still awaiting its DTO fetch', async () => { + // The gallery work awaits a DTO fetch, so a duplicate can land mid-flight. The handler must + // mark the event as processed before the first await, not after. + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await Promise.all([handler(buildImageCompleteEvent()), handler(buildImageCompleteEvent())]); + expect(getImageDTOSafe).toHaveBeenCalledTimes(1); + }); + + it('still processes distinct invocations of the same queue item', async () => { + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await handler(buildImageCompleteEvent()); + const secondNode = buildImageCompleteEvent(); + secondNode.invocation.id = 'prepared-node-2'; + await handler(secondNode); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + }); }); describe('buildOnForeignInvocationComplete', () => { diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx index 9bbb732b09e..20111fa8a7e 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx @@ -12,6 +12,7 @@ import { import { boardIdSelected, galleryViewChanged, imageSelected } from 'features/gallery/store/gallerySlice'; import { $nodeExecutionStates, upsertExecutionState } from 'features/nodes/hooks/useNodeExecutionState'; import { isImageField, isImageFieldCollection, isVideoField } from 'features/nodes/types/common'; +import { LRUCache } from 'lru-cache'; import type { ApiTagDescription } from 'services/api'; import { api, LIST_ALL_TAG, LIST_TAG } from 'services/api'; import { boardsApi } from 'services/api/endpoints/boards'; @@ -54,6 +55,16 @@ export const buildOnInvocationComplete = ( dispatch: AppDispatch, completedInvocationKeysByItemId: Map> ) => { + // A duplicate delivery of a completion event must not repeat the work below: re-running the + // gallery handling double-counts the optimistic board totals, and re-recording the auto-switch + // after its entry was consumed would suppress a later genuine gallery click on that image (see + // autoSwitchedImages). The shared completedInvocationKeysByItemId map cannot detect this — the + // workflow coordinator pre-marks first-delivery events for non-active workflow items before this + // handler runs — so the handler tracks what it has processed itself. The invocation id half of + // the key is the prepared node's per-execution UUID, so keys cannot collide across distinct + // executions even where item ids restart (in-memory DB); the LRU bounds memory. + const processedInvocations = new LRUCache({ max: 1000 }); + const addImagesToGallery = async (data: S['InvocationCompleteEvent']) => { if (nodeTypeDenylist.includes(data.invocation.type)) { log.trace(`Skipping denylisted node type (${data.invocation.type})`); @@ -360,6 +371,18 @@ export const buildOnInvocationComplete = ( return async (data: S['InvocationCompleteEvent']) => { log.debug({ data } as JsonObject, `Invocation complete (${data.invocation.type}, ${data.invocation_source_id})`); + const invocationKey = `${data.item_id}:${data.invocation.id}`; + if (processedInvocations.has(invocationKey)) { + log.trace( + { data } as JsonObject, + `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` + ); + return; + } + // Mark before the awaits below — a duplicate arriving while the DTO fetch is in flight must be + // rejected too. + processedInvocations.set(invocationKey, true); + const nodeExecutionState = $nodeExecutionStates.get()[data.invocation_source_id]; const updatedNodeExecutionState = getUpdatedNodeExecutionStateOnInvocationComplete( nodeExecutionState, From 6e263ff17164d225ca5ec7b93698f214ff679e42 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 21:06:11 -0400 Subject: [PATCH 5/8] =?UTF-8?q?fix(ui):=20address=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20selection-scoped=20auto-switch=20marker=20+=20looku?= =?UTF-8?q?p-failure=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of JPPhoto's findings applied to the current head (his third, the deadline-promotion blocker, was fixed ahead of this round by the merge of main's #9389: the lifecycle's onTerminal hands the shared preview to the freshest surviving session on every terminal status, and its test 'keeps promoting through a chain of terminations' pins his exact promote-B/cancel-B/C-still-active sequence). 1. The auto-switch marker is now scoped to the selection it was recorded for, not keyed by image name with a TTL. A redux listener settles the marker on every action that moves the gallery selection (matched by state change, not action type, so new selection-writing reducers are covered automatically). An auto-switch that never renders — because the user clicked elsewhere first, even without a rendered-image change — is dropped the moment the selection moves on, so it can never swallow the user's later click on that image. At most one marker exists, and only while its selection stands, so the TTL and pending bound are gone. 2. A completion delivery whose DTO lookups all fail no longer poisons the dedupe key: the key is dropped so a re-delivery can redo the gallery work instead of being turned away as a duplicate of a delivery that never landed. Partial failures keep the key — the fetched DTOs' board totals and optimistic inserts were already dispatched, and a retry would double-count them. Co-Authored-By: Claude Fable 5 --- .../listeners/autoSwitchedSelection.test.ts | 73 +++++++++ .../listeners/autoSwitchedSelection.ts | 27 ++++ invokeai/frontend/web/src/app/store/store.ts | 2 + .../ImageViewer/CurrentImagePreview.test.ts | 4 +- .../ImageViewer/CurrentImagePreview.tsx | 6 +- .../viewerProgressLifecycle.test.ts | 28 ++++ .../gallery/store/autoSwitchedImages.test.ts | 142 +++++++----------- .../gallery/store/autoSwitchedImages.ts | 88 +++++------ .../events/onInvocationComplete.test.ts | 73 +++++++++ .../services/events/onInvocationComplete.tsx | 87 ++++++++--- 10 files changed, 361 insertions(+), 169 deletions(-) create mode 100644 invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts create mode 100644 invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts new file mode 100644 index 00000000000..54c1ff2bde2 --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts @@ -0,0 +1,73 @@ +import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'; +import type { AppStartListening } from 'app/store/store'; +import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; +import { + boardIdSelected, + gallerySliceConfig, + imageSelected, + selectionChanged, +} from 'features/gallery/store/gallerySlice'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { addAutoSwitchedSelectionListener } from './autoSwitchedSelection'; + +// A store with the real gallery reducer and the real listener, so the predicate is exercised +// against actual selection-writing actions rather than a hand-built state pair. +const buildStore = () => { + const listenerMiddleware = createListenerMiddleware(); + addAutoSwitchedSelectionListener(listenerMiddleware.startListening as unknown as AppStartListening); + return configureStore({ + reducer: { gallery: gallerySliceConfig.slice.reducer }, + middleware: (getDefaultMiddleware) => getDefaultMiddleware().prepend(listenerMiddleware.middleware), + }); +}; + +describe('addAutoSwitchedSelectionListener', () => { + beforeEach(() => { + // The marker is a module singleton; drop anything a previous test left on it. + autoSwitchedImages.settle(null); + }); + + it('keeps the marker when the auto-switch selection lands', () => { + const store = buildStore(); + autoSwitchedImages.record('a.png'); + store.dispatch(imageSelected('a.png')); + expect(autoSwitchedImages.consume('a.png')).toBe(true); + }); + + it('drops the marker once the user selects something else', () => { + // The dead click this exists to prevent: the auto-switch to A never rendered because the user + // clicked B first, so their later click on A must still get its reveal. + const store = buildStore(); + autoSwitchedImages.record('a.png'); + store.dispatch(imageSelected('a.png')); + store.dispatch(imageSelected('b.png')); + store.dispatch(imageSelected('a.png')); + expect(autoSwitchedImages.consume('a.png')).toBe(false); + }); + + it('settles on every action that writes the selection, not just imageSelected', () => { + const store = buildStore(); + + autoSwitchedImages.record('a.png'); + store.dispatch(imageSelected('a.png')); + store.dispatch(selectionChanged(['b.png'])); + expect(autoSwitchedImages.consume('a.png')).toBe(false); + + autoSwitchedImages.record('c.png'); + store.dispatch(imageSelected('c.png')); + store.dispatch(boardIdSelected({ boardId: 'other', select: { selection: ['d.png'], galleryView: 'images' } })); + expect(autoSwitchedImages.consume('c.png')).toBe(false); + }); + + it('leaves the marker alone when an action does not move the selection', () => { + const store = buildStore(); + autoSwitchedImages.record('a.png'); + store.dispatch(imageSelected('a.png')); + // Selecting the same item again, and a board switch that carries no selection, must not + // discard a marker whose image has not rendered yet. + store.dispatch(imageSelected('a.png')); + store.dispatch(boardIdSelected({ boardId: 'other' })); + expect(autoSwitchedImages.consume('a.png')).toBe(true); + }); +}); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts new file mode 100644 index 00000000000..0b14616f0b3 --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts @@ -0,0 +1,27 @@ +import type { AppStartListening } from 'app/store/store'; +import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; +import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; + +/** + * Keeps the auto-switch marker scoped to the selection it was recorded for. + * + * onInvocationComplete records the item it is about to auto-switch to, so the viewer's reveal + * effect can tell that handoff apart from a user's gallery click. The marker is only meaningful + * while that selection stands: once the selection moves on, the recorded auto-switch will never + * render, and leaving the marker behind would make the user's next click on that item read as an + * auto-switch and get no reveal. + * + * Matched by state rather than by action type on purpose — the selection is written by several + * reducers (imageSelected, selectionChanged, boardIdSelected, comparedImagesSwapped, + * showVirtualBoardsChanged, logout), and a new one added later would silently escape an + * action-type list, leaving exactly the stale marker this exists to prevent. + */ +export const addAutoSwitchedSelectionListener = (startAppListening: AppStartListening) => { + startAppListening({ + predicate: (_action, currentState, previousState) => + selectLastSelectedItem(currentState) !== selectLastSelectedItem(previousState), + effect: (_action, { getState }) => { + autoSwitchedImages.settle(selectLastSelectedItem(getState()) ?? null); + }, + }); +}; diff --git a/invokeai/frontend/web/src/app/store/store.ts b/invokeai/frontend/web/src/app/store/store.ts index 59551225314..7ea4ffbf45c 100644 --- a/invokeai/frontend/web/src/app/store/store.ts +++ b/invokeai/frontend/web/src/app/store/store.ts @@ -12,6 +12,7 @@ import { errorHandler } from 'app/store/enhancers/reduxRemember/errors'; import { addAdHocPostProcessingRequestedListener } from 'app/store/middleware/listenerMiddleware/listeners/addAdHocPostProcessingRequestedListener'; import { addAnyEnqueuedListener } from 'app/store/middleware/listenerMiddleware/listeners/anyEnqueued'; import { addAppStartedListener } from 'app/store/middleware/listenerMiddleware/listeners/appStarted'; +import { addAutoSwitchedSelectionListener } from 'app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection'; import { addBatchEnqueuedListener } from 'app/store/middleware/listenerMiddleware/listeners/batchEnqueued'; import { addDeleteBoardAndImagesFulfilledListener } from 'app/store/middleware/listenerMiddleware/listeners/boardAndImagesDeleted'; import { addBoardIdSelectedListener } from 'app/store/middleware/listenerMiddleware/listeners/boardIdSelected'; @@ -326,6 +327,7 @@ addImageAddedToBoardFulfilledListener(startAppListening); addImageRemovedFromBoardFulfilledListener(startAppListening); addBoardIdSelectedListener(startAppListening); addArchivedOrDeletedBoardListener(startAppListening); +addAutoSwitchedSelectionListener(startAppListening); // Node schemas addGetOpenAPISchemaListener(startAppListening); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts index 00ec4804ffd..64975a7fbf9 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts @@ -30,8 +30,8 @@ describe('CurrentImagePreview reveal wiring', () => { expect(onReady).toContain('onLoadImage(imageDTO.session_id ?? null)'); }); - it('settles the auto-switch registry on every rendered-image change', () => { - // The consume call must sit before the reveal gates, so entries are settled even when the + it('consumes the auto-switch marker on every rendered-image change', () => { + // The consume call must sit before the reveal gates, so the marker is cleared even when the // auto-switched image renders with no progress showing. const revealEffect = currentImagePreview.slice( currentImagePreview.indexOf('const renderedImageName ='), diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx index 815aaeaeacb..64bb9d12588 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -119,8 +119,10 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu previousRenderedImageNameRef.current = renderedImageName; // Consume on every change of the rendered image, not only when the reveal conditions below - // hold — each render settles the registry (see autoSwitchedImages.consume), so no stale entry - // survives to suppress a genuine user selection of the same image later. + // hold — in the common case the auto-switched image renders with no progress showing, and the + // marker must not outlive the render it was recorded for. The marker is also dropped whenever + // the selection moves on before rendering (see addAutoSwitchedSelectionListener), so it can + // never suppress a genuine user selection of the same image later. const wasAutoSwitchedTo = renderedImageName !== null && renderedImageName !== previousRenderedImageName && diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.test.ts index a3181382a7f..a807d9ba7d3 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.test.ts @@ -130,6 +130,34 @@ describe('viewerProgressLifecycle', () => { expect(stores.$progressEvent.get()?.item_id).toBe(1); }); + it('keeps promoting through a chain of terminations while any session is still generating', () => { + // Three sessions, each promotion making the next owner: the shared preview must never be + // torn down while something is still generating, no matter how many owners terminate. Each + // terminal event here arrives with no further progress event behind it, so a promotion that + // left the new owner unprotected would show up as a blank viewer on the next termination. + const eventB = buildProgressEvent({ item_id: 2, session_id: 'session-2', image: buildProgressImage(2) }); + const eventC = buildProgressEvent({ item_id: 3, session_id: 'session-3', image: buildProgressImage(3) }); + const eventA = buildProgressEvent({ item_id: 1, session_id: 'session-1', image: buildProgressImage(1) }); + lifecycle.recordProgress(eventC); + lifecycle.recordProgress(eventB); + lifecycle.recordProgress(eventA); + + // A owns the preview and completes: B reported most recently of the two left, so B takes it. + lifecycle.onTerminal(buildTerminalEvent({ item_id: 1, status: 'completed' }), true); + expect(stores.$progressEvent.get()).toBe(eventB); + + // B is then canceled while C is still generating — C must take the preview, not lose it. + lifecycle.onTerminal(buildTerminalEvent({ item_id: 2, status: 'canceled' }), true); + expect(stores.$progressEvent.get()).toBe(eventC); + expect(stores.$progressImage.get()).toBe(eventC.image); + expect(stores.$isProgressImageResolving.get()).toBe(false); + + // Only when the last session ends does the preview come down. + lifecycle.onTerminal(buildTerminalEvent({ item_id: 3, status: 'canceled' }), true); + expect(stores.$progressEvent.get()).toBeNull(); + expect(stores.$progressImage.get()).toBeNull(); + }); + it('leaves the shared preview alone when a non-owner terminates', () => { const { eventA } = startTwoSessions(); lifecycle.onTerminal(buildTerminalEvent({ item_id: 2, status: 'canceled' }), true); diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts index da00b54bce3..6e28810a2cd 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts @@ -1,107 +1,69 @@ import { describe, expect, it } from 'vitest'; -import { createAutoSwitchedImageRegistry } from './autoSwitchedImages'; +import { createAutoSwitchedSelectionMarker } from './autoSwitchedImages'; -describe('createAutoSwitchedImageRegistry', () => { - it('consumes a recorded name exactly once', () => { - const registry = createAutoSwitchedImageRegistry(); - registry.record('a.png'); - expect(registry.consume('a.png')).toBe(true); - expect(registry.consume('a.png')).toBe(false); +describe('createAutoSwitchedSelectionMarker', () => { + it('reports the auto-switched item on its first render, once', () => { + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + expect(marker.consume('a.png')).toBe(true); + expect(marker.consume('a.png')).toBe(false); }); - it('returns false for a name that was never recorded', () => { - const registry = createAutoSwitchedImageRegistry(); - expect(registry.consume('a.png')).toBe(false); + it('reports nothing for an item that was never auto-switched to', () => { + const marker = createAutoSwitchedSelectionMarker(); + expect(marker.consume('a.png')).toBe(false); }); - it('consumes entries that render in record order', () => { - const registry = createAutoSwitchedImageRegistry(); - registry.record('a.png'); - registry.record('b.png'); - expect(registry.consume('a.png')).toBe(true); - expect(registry.consume('b.png')).toBe(true); - expect(registry.consume('b.png')).toBe(false); + it('drops a marker whose selection was superseded before it rendered', () => { + // The auto-switch to A is dispatched, then the user clicks B before A's preload settles. A can + // never render as that auto-switch, and the user's later click on A must reveal it. + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + marker.settle('b.png'); + marker.settle('a.png'); + expect(marker.consume('a.png')).toBe(false); }); - it('drops entries recorded before the consumed one — their selections were superseded', () => { - // Two completions within one thumbnail-fetch window: only b renders. If a's entry survived, - // it would suppress a genuine user click on a during the next generation. - const registry = createAutoSwitchedImageRegistry(); - registry.record('a.png'); - registry.record('b.png'); - expect(registry.consume('b.png')).toBe(true); - expect(registry.consume('a.png')).toBe(false); + it('keeps only the last of several auto-switches settled in one batch', () => { + // Two sessions completing together record two names, but only the last selection stands. + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + marker.record('b.png'); + marker.settle('b.png'); + expect(marker.consume('b.png')).toBe(true); + expect(marker.consume('a.png')).toBe(false); }); - it('clears all pending entries when an unrecorded image renders', () => { - // A rendered image that was never recorded is user activity: any pending selections have been - // superseded and will never first-render, so their entries must not linger to swallow a later - // genuine click. - const registry = createAutoSwitchedImageRegistry(); - registry.record('a.png'); - expect(registry.consume('user-click.png')).toBe(false); - registry.record('b.png'); - expect(registry.consume('a.png')).toBe(false); - // The miss above also settled b.png away. - expect(registry.consume('b.png')).toBe(false); + it('survives settles that do not move the selection', () => { + // Re-selecting the same item (or any other action that leaves the selection alone) must not + // discard a marker whose item has not rendered yet. + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + marker.settle('a.png'); + expect(marker.consume('a.png')).toBe(true); }); - it('lets a genuine re-selection reveal after a stale entry is settled by a later render', () => { - // JPPhoto's review sequence: a is recorded again after it already rendered (stale entry), then - // the user selects b -> a during the next generation. The b render must settle the stale entry - // so the click on a reveals. - const registry = createAutoSwitchedImageRegistry(); - registry.record('a.png'); - expect(registry.consume('a.png')).toBe(true); // auto-switch renders a - registry.record('a.png'); // stale re-record - expect(registry.consume('b.png')).toBe(false); // user clicks b — settles the registry - expect(registry.consume('a.png')).toBe(false); // user clicks a — reveal must fire + it('drops the marker when the selection is cleared', () => { + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + marker.settle(null); + expect(marker.consume('a.png')).toBe(false); }); - it('evicts the oldest entry beyond the bound', () => { - const registry = createAutoSwitchedImageRegistry(); - for (let i = 0; i < 9; i++) { - registry.record(`image-${i}.png`); - } - // 9 recorded, bound is 8 — the oldest is gone. - expect(registry.consume('image-0.png')).toBe(false); - }); - - it('retains the newest entries under the bound', () => { - const registry = createAutoSwitchedImageRegistry(); - for (let i = 0; i < 9; i++) { - registry.record(`image-${i}.png`); - } - // image-0 was evicted by the bound, so image-1 is the oldest survivor. - expect(registry.consume('image-1.png')).toBe(true); - expect(registry.consume('image-8.png')).toBe(true); - }); - - it('expires entries after the TTL', () => { - let t = 0; - const registry = createAutoSwitchedImageRegistry(() => t); - registry.record('a.png'); - t = 30_001; - expect(registry.consume('a.png')).toBe(false); - }); - - it('keeps entries up to the TTL boundary', () => { - let t = 0; - const registry = createAutoSwitchedImageRegistry(() => t); - registry.record('a.png'); - t = 30_000; - expect(registry.consume('a.png')).toBe(true); - }); - - it('prunes expired entries without touching live ones', () => { - let t = 0; - const registry = createAutoSwitchedImageRegistry(() => t); - registry.record('old.png'); - t = 20_000; - registry.record('new.png'); - t = 40_000; - // old.png has expired; new.png is still live and consumable. - expect(registry.consume('new.png')).toBe(true); + it('does not report a second render of an item after its marker was consumed', () => { + // The user navigates away and back: that second render is a user selection and must reveal. + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + expect(marker.consume('a.png')).toBe(true); + marker.settle('b.png'); + marker.settle('a.png'); + expect(marker.consume('a.png')).toBe(false); }); }); diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts index b6cd2e77472..d7ab9003458 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.ts @@ -1,7 +1,7 @@ /** - * Names of images the gallery auto-switched to, pending their first render in the viewer. + * Marks the gallery selection the auto-switch made, until the viewer renders it. * - * The viewer briefly reveals a newly selected image over the progress overlay so that a + * The viewer briefly reveals a newly selected item over the progress overlay so that a * mid-generation gallery click is not invisible (see the reveal effect in CurrentImagePreview). * Auto-switch selections must not trigger that reveal — but they land asynchronously: the switch is * dispatched only after onInvocationComplete's DTO fetch resolves, and the viewer renders it only @@ -10,70 +10,52 @@ * time the auto-switch selection reaches the viewer it is indistinguishable from a user click and * the reveal flashes the previous result over the live preview for 2 seconds. * - * Recording the image name at dispatch and consuming it on the selection's first render - * distinguishes the two without depending on event timing. + * The marker is scoped to the selection it was recorded for, not to the item name: it survives only + * as long as that selection stands. A name-keyed marker cannot tell "this render is the auto-switch + * landing" from "the user picked that same item later", so an auto-switch that never rendered — + * because the user clicked elsewhere first — would swallow their later click on it, the very dead + * click the reveal exists to prevent. Settling on every selection change closes that: once the + * selection moves on, the recorded auto-switch can never render, and the marker goes with it. + * + * That scoping is also why no expiry or bound is needed. At most one marker exists, and only while + * its selection is the current one. */ -type AutoSwitchedImageRegistry = { - /** Records that the gallery is auto-switching to this image. */ - record: (imageName: string) => void; +type AutoSwitchedSelectionMarker = { + /** Records that the gallery is auto-switching the selection to this item. Call immediately + * before dispatching that selection, so the settle it triggers sees the marker. */ + record: (itemName: string) => void; + /** + * Points the marker at the selection that now stands, dropping it unless it is still the one + * recorded. Call on every selection change (see addAutoSwitchedSelectionListener). + */ + settle: (selectedItemName: string | null) => void; /** - * Returns whether this image was recently auto-switched to. Call exactly once per rendered-image - * change: every call settles the registry, because each render proves what became of the pending - * selections. A match removes the entry and every entry recorded before it — those selections - * were superseded and will never first-render (e.g. two completions within one thumbnail-fetch - * window; only the last one renders). A miss means an image that was never recorded rendered, - * i.e. user activity superseded every pending selection, so the registry is cleared entirely. - * Either way, no entry survives past the next rendered-image change to swallow a genuine user - * click later — the very dead-click the reveal exists to prevent. - * - * The miss-clear is deliberately over-eager: a user click made *before* an entry was recorded - * can render *after* it (its preload was already in flight), wiping that live entry and - * readmitting one 2-second flash. That interleave is narrow, and trading it for stale entries - * that swallow clicks would be backwards — the flash is the milder failure. + * Returns whether the item now rendering is the one the auto-switch selected, clearing the + * marker. Call on every change of the rendered item. */ - consume: (imageName: string) => boolean; + consume: (itemName: string) => boolean; }; -// consume settles the registry on every rendered-image change, so entries can only accumulate -// while nothing renders at all — the viewer unmounted by comparison mode while generations keep -// completing. The bound caps memory there; a dropped entry's worst case is one spurious 2-second -// reveal. -const MAX_PENDING = 8; - -// Same no-renders window as above: an entry is only meaningful between the auto-switch dispatch -// and the image's first render, and with the viewer unmounted that render may never come. Without -// the TTL, remounting the viewer within reach of such an orphan and clicking its image would -// suppress the reveal. Generous enough for a slow thumbnail fetch on a bad connection; expiring -// early merely readmits the 2-second flash there, which is the milder failure. -const TTL_MS = 30_000; - -export const createAutoSwitchedImageRegistry = (now: () => number = Date.now): AutoSwitchedImageRegistry => { - let pending: { imageName: string; recordedAt: number }[] = []; - - const prune = () => { - const cutoff = now() - TTL_MS; - pending = pending.filter((entry) => entry.recordedAt >= cutoff); - }; +export const createAutoSwitchedSelectionMarker = (): AutoSwitchedSelectionMarker => { + let pendingItemName: string | null = null; return { - record: (imageName) => { - prune(); - pending.push({ imageName, recordedAt: now() }); - if (pending.length > MAX_PENDING) { - pending.shift(); + record: (itemName) => { + pendingItemName = itemName; + }, + settle: (selectedItemName) => { + if (pendingItemName !== selectedItemName) { + pendingItemName = null; } }, - consume: (imageName) => { - prune(); - const index = pending.findIndex((entry) => entry.imageName === imageName); - if (index === -1) { - pending = []; + consume: (itemName) => { + if (pendingItemName !== itemName) { return false; } - pending = pending.slice(index + 1); + pendingItemName = null; return true; }, }; }; -export const autoSwitchedImages = createAutoSwitchedImageRegistry(); +export const autoSwitchedImages = createAutoSwitchedSelectionMarker(); diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts index 66467d06559..a3208fafa2d 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts @@ -331,6 +331,79 @@ describe('onInvocationComplete polymorphic gallery cache', () => { expect(getImageDTOSafe).toHaveBeenCalledTimes(1); }); + it('lets a re-delivery retry when the first attempt lost its DTO to a transient failure', async () => { + // getImageDTOSafe swallows fetch errors and returns null, so a transient failure silently + // drops the image from the gallery. Keeping the dedupe key in that case would make the loss + // permanent — the redelivery that could fix it is turned away as a duplicate. + vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); + + const dispatched: unknown[] = []; + const dispatch = vi.fn((action: unknown) => { + dispatched.push(action); + return { unwrap: () => Promise.resolve(undefined) }; + }); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await handler(buildImageCompleteEvent()); + expect( + dispatched.some((action) => { + const payload = (action as { payload?: unknown }).payload; + return Array.isArray(payload) && payload.includes('GalleryItemNameList'); + }), + 'a failed lookup produces no gallery work' + ).toBe(false); + + await handler(buildImageCompleteEvent()); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + expect( + dispatched.some((action) => { + const payload = (action as { payload?: unknown }).payload; + return Array.isArray(payload) && payload.includes('GalleryItemNameList'); + }), + 'the retry must do the gallery work the first delivery lost' + ).toBe(true); + }); + + it('does not retry a partial failure — the fetched DTOs were already dispatched', async () => { + // One of two image lookups fails: the successful one's board totals and optimistic insert have + // already gone out, so a re-delivery re-running the gallery work would double-count them. The + // dedupe key is only dropped when NOTHING was fetched. + vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const twoImages = buildImageCompleteEvent(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (twoImages as any).result = { + image_1: { image_name: 'first.png' }, + image_2: { image_name: 'second.png' }, + }; + + await handler(twoImages); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + + // The re-delivery must be treated as a duplicate — no further lookups. + await handler(twoImages); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + }); + it('still processes distinct invocations of the same queue item', async () => { const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); const getState = vi.fn(() => ({})); diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx index 20111fa8a7e..a2d03efc1e6 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx @@ -56,24 +56,29 @@ export const buildOnInvocationComplete = ( completedInvocationKeysByItemId: Map> ) => { // A duplicate delivery of a completion event must not repeat the work below: re-running the - // gallery handling double-counts the optimistic board totals, and re-recording the auto-switch - // after its entry was consumed would suppress a later genuine gallery click on that image (see - // autoSwitchedImages). The shared completedInvocationKeysByItemId map cannot detect this — the + // gallery handling double-counts the optimistic board totals and re-dispatches the auto-switch + // selection. The shared completedInvocationKeysByItemId map cannot detect this — the // workflow coordinator pre-marks first-delivery events for non-active workflow items before this // handler runs — so the handler tracks what it has processed itself. The invocation id half of // the key is the prepared node's per-execution UUID, so keys cannot collide across distinct // executions even where item ids restart (in-memory DB); the LRU bounds memory. const processedInvocations = new LRUCache({ max: 1000 }); - const addImagesToGallery = async (data: S['InvocationCompleteEvent']) => { + // Returns how many image DTOs were fetched, so the caller can tell "this event had no gallery + // output" apart from "the output was there but every lookup failed" (see the dedupe-key drop in + // the handler below). + const addImagesToGallery = async ( + data: S['InvocationCompleteEvent'], + onLookupFailure: () => void + ): Promise => { if (nodeTypeDenylist.includes(data.invocation.type)) { log.trace(`Skipping denylisted node type (${data.invocation.type})`); - return; + return 0; } - const imageDTOs = await getResultImageDTOs(data); + const imageDTOs = await getResultImageDTOs(data, onLookupFailure); if (imageDTOs.length === 0) { - return; + return 0; } // For efficiency's sake, we want to minimize the number of dispatches and invalidations we do. @@ -83,7 +88,7 @@ export const buildOnInvocationComplete = ( for (const imageDTO of imageDTOs) { if (imageDTO.is_intermediate) { - return; + return imageDTOs.length; } const board_id = imageDTO.board_id ?? 'none'; @@ -195,14 +200,14 @@ export const buildOnInvocationComplete = ( const autoSwitch = selectAutoSwitch(getState()); if (!autoSwitch) { - return; + return imageDTOs.length; } // Finally, we may need to autoswitch to the new image. We'll only do it for the last image in the list. const lastImageDTO = imageDTOs.at(-1); if (!lastImageDTO) { - return; + return imageDTOs.length; } const { image_name } = lastImageDTO; @@ -238,9 +243,18 @@ export const buildOnInvocationComplete = ( // Select the image immediately since we've optimistically updated the cache dispatch(imageSelected(lastImageDTO.image_name)); } + + return imageDTOs.length; }; - const getResultImageDTOs = async (data: S['InvocationCompleteEvent']): Promise => { + // getImageDTOSafe swallows fetch errors and returns null, which downstream is indistinguishable + // from "this node produced no image". onLookupFailure separates the two: the handler drops this + // event's dedupe key when it fires, so a re-delivery redoes the gallery work instead of being + // turned away as a duplicate of a delivery whose output never reached the gallery. + const getResultImageDTOs = async ( + data: S['InvocationCompleteEvent'], + onLookupFailure: () => void + ): Promise => { const { result } = data; const imageDTOs: ImageDTO[] = []; for (const [_name, value] of objectEntries(result)) { @@ -248,12 +262,16 @@ export const buildOnInvocationComplete = ( const imageDTO = await getImageDTOSafe(value.image_name); if (imageDTO) { imageDTOs.push(imageDTO); + } else { + onLookupFailure(); } } else if (isImageFieldCollection(value)) { for (const imageField of value) { const imageDTO = await getImageDTOSafe(imageField.image_name); if (imageDTO) { imageDTOs.push(imageDTO); + } else { + onLookupFailure(); } } } @@ -261,7 +279,10 @@ export const buildOnInvocationComplete = ( return imageDTOs; }; - const getResultVideoDTOs = async (data: S['InvocationCompleteEvent']): Promise => { + const getResultVideoDTOs = async ( + data: S['InvocationCompleteEvent'], + onLookupFailure: () => void + ): Promise => { const { result } = data; const videoDTOs: VideoDTO[] = []; for (const [_name, value] of objectEntries(result)) { @@ -269,6 +290,8 @@ export const buildOnInvocationComplete = ( const videoDTO = await getVideoDTOSafe(value.video_name); if (videoDTO) { videoDTOs.push(videoDTO); + } else { + onLookupFailure(); } } } @@ -284,19 +307,22 @@ export const buildOnInvocationComplete = ( // (DndImage onLoad) to clear them. When auto-switching to a video, the viewer swaps // CurrentImagePreview for CurrentVideoPreview, which unmounts the stale progress overlay // so the stuck "Saving video" spinner goes away on its own. - const addVideosToGallery = async (data: S['InvocationCompleteEvent']) => { + const addVideosToGallery = async ( + data: S['InvocationCompleteEvent'], + onLookupFailure: () => void + ): Promise => { if (nodeTypeDenylist.includes(data.invocation.type)) { - return; + return 0; } - const videoDTOs = await getResultVideoDTOs(data); + const videoDTOs = await getResultVideoDTOs(data, onLookupFailure); if (videoDTOs.length === 0) { - return; + return 0; } const nonIntermediate = videoDTOs.filter((v) => !v.is_intermediate); if (nonIntermediate.length === 0) { - return; + return videoDTOs.length; } // Force the polymorphic gallery list to refetch so the new video shows up. Note: this is @@ -315,12 +341,12 @@ export const buildOnInvocationComplete = ( const autoSwitch = selectAutoSwitch(getState()); if (!autoSwitch) { - return; + return videoDTOs.length; } const lastVideoDTO = nonIntermediate.at(-1); if (!lastVideoDTO) { - return; + return videoDTOs.length; } const { video_name } = lastVideoDTO; @@ -345,6 +371,8 @@ export const buildOnInvocationComplete = ( } dispatch(imageSelected(video_name)); } + + return videoDTOs.length; }; const clearCanvasWorkflowIntegrationProcessing = (data: S['InvocationCompleteEvent']) => { @@ -380,8 +408,14 @@ export const buildOnInvocationComplete = ( return; } // Mark before the awaits below — a duplicate arriving while the DTO fetch is in flight must be - // rejected too. + // rejected too. Dropped again below when every DTO lookup failed: the gallery never got any of + // that output, so a re-delivery must be free to redo the work rather than be turned away as a + // duplicate of a delivery that never landed. processedInvocations.set(invocationKey, true); + let hadLookupFailure = false; + const onLookupFailure = () => { + hadLookupFailure = true; + }; const nodeExecutionState = $nodeExecutionStates.get()[data.invocation_source_id]; const updatedNodeExecutionState = getUpdatedNodeExecutionStateOnInvocationComplete( @@ -405,10 +439,19 @@ export const buildOnInvocationComplete = ( clearCanvasWorkflowIntegrationProcessing(data); // Add images to gallery (canvas workflow integration results go to staging area automatically) - await addImagesToGallery(data); - await addVideosToGallery(data); + const fetchedImageCount = await addImagesToGallery(data, onLookupFailure); + const fetchedVideoCount = await addVideosToGallery(data, onLookupFailure); $lastProgressEvent.set(null); + + // Drop the dedupe key only when a lookup failed AND nothing was fetched: such a delivery + // dispatched no gallery work at all, so a re-delivery is free to redo everything (the + // node-execution upsert has its own dedupe via completedInvocationKeysByItemId). A partial + // failure keeps the key — the fetched DTOs' board totals and optimistic inserts were already + // dispatched, and a re-delivery re-running them would double-count. + if (hadLookupFailure && fetchedImageCount + fetchedVideoCount === 0) { + processedInvocations.delete(invocationKey); + } }; }; From f407cdc38e4ec758a2b900a0327b64455ce9c87e Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 21:32:03 -0400 Subject: [PATCH 6/8] test(ui): cover the reveal suppression, and narrow the retry to gallery work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of the round-3 changes found the mechanism sound but its verification hollow: deleting the auto-switch record(), or making the suppression branch unreachable, left all 1888 tests green. Both mutations remove the behavior this PR exists to deliver. - The reveal decision moves into a pure getSelectedItemRevealDecision, unit tested branch by branch. It answers 'reveal' or 'hide' and nothing else: the caller clears the running reveal's timer before asking, so a path that returned without writing the atom would strand the reveal on with no timer left to end it. That was reachable in the old shape via the previous-name early return. - onInvocationComplete gains tests that the auto-switched selection is actually marked (and is not when auto-switch is off), and that the video half of the retry condition is load-bearing, via a mixed image+video result. Two lower-severity findings from the same review: - A retry re-ran the whole handler, including two side effects that are global rather than per-event: the canvas processing flag and $lastProgressEvent. A re-delivery arriving after the user started another run would stop that run's spinner and blank its progress. The dedupe entry now records what is outstanding ('done' vs 'gallery-retryable') and a retry redoes only the gallery work. - The retry condition read "DTOs fetched", but two paths return before dispatching anything — a first intermediate image, and an all- intermediate video result. An event whose surviving output was intermediate therefore kept its key with nothing dispatched, so the lookup that failed alongside it could never be retried. The counts now mean "gallery work dispatched". Every fix above is pinned by a test that fails without it. --- .../ImageViewer/CurrentImagePreview.test.ts | 11 ++ .../ImageViewer/CurrentImagePreview.tsx | 36 ++-- .../ImageViewer/selectedItemReveal.test.ts | 69 +++++++ .../ImageViewer/selectedItemReveal.ts | 74 ++++++++ .../events/onInvocationComplete.test.ts | 169 ++++++++++++++++++ .../services/events/onInvocationComplete.tsx | 93 +++++----- 6 files changed, 391 insertions(+), 61 deletions(-) create mode 100644 invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts create mode 100644 invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts index 64975a7fbf9..73c81cab943 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts @@ -30,6 +30,17 @@ describe('CurrentImagePreview reveal wiring', () => { expect(onReady).toContain('onLoadImage(imageDTO.session_id ?? null)'); }); + it('routes the reveal through the shared decision, passing the auto-switch marker to it', () => { + // The decision's branches are unit tested in selectedItemReveal.test.ts; what cannot be seen + // from there is whether this component still feeds it the marker, or still writes the atom on + // the hide path. + expect(currentImagePreview).toContain('getSelectedItemRevealDecision({'); + expect(currentImagePreview).toContain('wasAutoSwitchedTo,'); + expect(currentImagePreview).toMatch( + /if \(decision === 'hide'\) \{\s+\$isTemporarilyShowingSelectedImage\.set\(false\);\s+return;/ + ); + }); + it('consumes the auto-switch marker on every rendered-image change', () => { // The consume call must sit before the reveal gates, so the marker is cleared even when the // auto-switched image renders with no progress showing. diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx index 64bb9d12588..55688c037bc 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -26,6 +26,7 @@ import { NoContentForViewer } from './NoContentForViewer'; import { ProgressImage } from './ProgressImage2'; import { ProgressImageTiles } from './ProgressImageTiles'; import { ProgressIndicator } from './ProgressIndicator2'; +import { getSelectedItemRevealDecision } from './selectedItemReveal'; export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | null }) => { const activeTab = useAppSelector(selectActiveTab); @@ -128,29 +129,22 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu renderedImageName !== previousRenderedImageName && autoSwitchedImages.consume(renderedImageName); + // Clearing the timer before deciding is why getSelectedItemRevealDecision has no third + // "leave it alone" outcome: a path that returned without writing the atom would leave a raised + // reveal with nothing left to lower it, hiding the live preview for the rest of the render. window.clearTimeout(selectedImageRevealTimeoutId.current); - if ( - !shouldShowProgressInViewer || - !hasProgressImage || - isProgressImageResolving || - !renderedImageName || - renderedImageName !== selectedImageName - ) { - $isTemporarilyShowingSelectedImage.set(false); - return; - } - - if (previousRenderedImageName === null || previousRenderedImageName === renderedImageName) { - return; - } - - // The reveal exists to make a mid-generation *user* selection visible. An auto-switch to a - // just-finished image can land here late — after the next generation's first progress event - // has already reset $isProgressImageResolving — and must not flash the previous result over - // the live preview. The set(false) is required: the clearTimeout above already cancelled any - // running reveal's timer, so returning with the atom still true would wedge the reveal on. - if (wasAutoSwitchedTo) { + const decision = getSelectedItemRevealDecision({ + shouldShowProgressInViewer, + hasProgressImage, + isProgressImageResolving, + renderedItemName: renderedImageName, + selectedItemName: selectedImageName ?? null, + previousRenderedItemName: previousRenderedImageName, + wasAutoSwitchedTo, + }); + + if (decision === 'hide') { $isTemporarilyShowingSelectedImage.set(false); return; } diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts new file mode 100644 index 00000000000..b9228abe693 --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from 'vitest'; + +import type { SelectedItemRevealInputs } from './selectedItemReveal'; +import { getSelectedItemRevealDecision } from './selectedItemReveal'; + +// A mid-generation user click on b.png, with the previous item still remembered: the case the +// reveal exists for. Each test below perturbs exactly one input away from it. +const userClickMidGeneration: SelectedItemRevealInputs = { + shouldShowProgressInViewer: true, + hasProgressImage: true, + isProgressImageResolving: false, + renderedItemName: 'b.png', + selectedItemName: 'b.png', + previousRenderedItemName: 'a.png', + wasAutoSwitchedTo: false, +}; + +describe('getSelectedItemRevealDecision', () => { + it('reveals a mid-generation user click so it visibly lands', () => { + expect(getSelectedItemRevealDecision(userClickMidGeneration)).toBe('reveal'); + }); + + it('does not reveal an auto-switch to a just-finished item', () => { + // Without this the finished image flashes over the next generation's live preview for 2s. + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, wasAutoSwitchedTo: true })).toBe('hide'); + }); + + it('hides when no progress preview is covering the viewer', () => { + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, hasProgressImage: false })).toBe('hide'); + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, shouldShowProgressInViewer: false })).toBe( + 'hide' + ); + }); + + it('hides while a finished generation is resolving into its final image', () => { + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, isProgressImageResolving: true })).toBe('hide'); + }); + + it('hides while the render still lags the selection', () => { + // The preload has not settled, so the item on screen is not the one that was clicked. + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, renderedItemName: 'a.png' })).toBe('hide'); + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, renderedItemName: null })).toBe('hide'); + }); + + it('hides when the displayed item did not change', () => { + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, previousRenderedItemName: 'b.png' })).toBe( + 'hide' + ); + }); + + it('hides on the first render, which is not a click', () => { + expect(getSelectedItemRevealDecision({ ...userClickMidGeneration, previousRenderedItemName: null })).toBe('hide'); + }); + + it('never answers anything but reveal or hide', () => { + // The caller has already cleared the running reveal's timer by the time it asks, so a third + // "leave it alone" outcome would wedge the overlay off for the rest of the render. + const inputs: SelectedItemRevealInputs[] = [ + userClickMidGeneration, + { ...userClickMidGeneration, wasAutoSwitchedTo: true }, + { ...userClickMidGeneration, hasProgressImage: false }, + { ...userClickMidGeneration, renderedItemName: null }, + { ...userClickMidGeneration, previousRenderedItemName: null }, + ]; + for (const input of inputs) { + expect(['reveal', 'hide']).toContain(getSelectedItemRevealDecision(input)); + } + }); +}); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts new file mode 100644 index 00000000000..91b7a599ebf --- /dev/null +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts @@ -0,0 +1,74 @@ +/** + * Whether the viewer should lift its progress overlay to show the selected item. + * + * A generation covers the viewer with an opaque progress overlay, which would otherwise swallow + * every gallery click for the whole render: the selection changes underneath, but nothing visibly + * happens. The reveal makes that click land — the clicked item is shown for + * SELECTED_ITEM_REVEAL_DURATION_MS, then the live preview returns. + * + * Only two outcomes, deliberately. The caller clears the running reveal's timer before asking, so a + * "leave it as it is" answer would strand the reveal on for the rest of the render with no timer + * left to end it — the decision must always name the state the overlay ends up in. + */ +export type SelectedItemRevealDecision = 'reveal' | 'hide'; + +export type SelectedItemRevealInputs = { + /** The user's "show progress in viewer" setting. */ + shouldShowProgressInViewer: boolean; + /** Whether a live progress preview exists to be covering the viewer at all. */ + hasProgressImage: boolean; + /** Whether a finished generation's preview is mid-handoff to its final image. */ + isProgressImageResolving: boolean; + /** The item the viewer is rendering right now, or null when it has nothing to show. */ + renderedItemName: string | null; + /** The item the gallery selection points at. */ + selectedItemName: string | null; + /** The item the viewer rendered before this change — null on the first render. */ + previousRenderedItemName: string | null; + /** Whether this render is the gallery auto-switching to a just-finished item, rather than the + * user picking one. See features/gallery/store/autoSwitchedImages. */ + wasAutoSwitchedTo: boolean; +}; + +export const getSelectedItemRevealDecision = ({ + shouldShowProgressInViewer, + hasProgressImage, + isProgressImageResolving, + renderedItemName, + selectedItemName, + previousRenderedItemName, + wasAutoSwitchedTo, +}: SelectedItemRevealInputs): SelectedItemRevealDecision => { + // Nothing is covering the viewer, so there is nothing to reveal from under. + if (!shouldShowProgressInViewer || !hasProgressImage) { + return 'hide'; + } + + // The preview is already resolving into a finished generation's final image; a reveal here would + // fight that handoff. + if (isProgressImageResolving) { + return 'hide'; + } + + // Render lagging the selection (the preload has not settled yet): whatever is on screen is not + // what the user picked, so showing it would not make their click land. + if (renderedItemName === null || renderedItemName !== selectedItemName) { + return 'hide'; + } + + // Not a change of displayed item — nothing happened that needs to be made visible. The first + // render after the viewer opens is not a click either. + if (previousRenderedItemName === null || previousRenderedItemName === renderedItemName) { + return 'hide'; + } + + // The reveal exists for *user* selections. An auto-switch to a just-finished item lands + // asynchronously and can reach the viewer after the next generation's first progress event, so + // timing cannot tell it apart from a click — treating it as one flashes the finished item over + // the new generation's live preview for two seconds. + if (wasAutoSwitchedTo) { + return 'hide'; + } + + return 'reveal'; +}; diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts index a3208fafa2d..b89ed0ca406 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts @@ -117,8 +117,11 @@ vi.mock('services/events/stores', () => ({ // Import AFTER the mocks above are declared (vi.mock is hoisted; explicit ordering here // is for the human reader). +import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; +import { selectAutoSwitch } from 'features/gallery/store/gallerySelectors'; import { getImageDTOSafe } from 'services/api/endpoints/images'; import { getVideoDTOSafe } from 'services/api/endpoints/videos'; +import { $lastProgressEvent } from 'services/events/stores'; import { buildOnForeignInvocationComplete, @@ -157,6 +160,10 @@ const buildImageCompleteEvent = (): S['InvocationCompleteEvent'] => describe('onInvocationComplete polymorphic gallery cache', () => { beforeEach(() => { vi.clearAllMocks(); + // clearAllMocks resets calls, not implementations — restore the factory default so a test that + // turns auto-switch on cannot leak it into the next one. + vi.mocked(selectAutoSwitch).mockReturnValue(false); + autoSwitchedImages.settle(null); }); it('invalidates GalleryItemNameList + GalleryItemList when an image output completes', async () => { @@ -404,6 +411,168 @@ describe('onInvocationComplete polymorphic gallery cache', () => { expect(getImageDTOSafe).toHaveBeenCalledTimes(2); }); + it('marks the selection it auto-switches to, so the viewer does not reveal it as a user click', async () => { + // Without the marker the finished image flashes over the next generation's live preview for + // two seconds — the regression this PR exists to fix. The marker is the only signal that can + // tell the handoff from a click, since it lands after the next generation's first progress + // event has already reset the timing-based guard. + vi.mocked(selectAutoSwitch).mockReturnValue(true); + autoSwitchedImages.settle(null); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await handler(buildImageCompleteEvent()); + + expect(autoSwitchedImages.consume('fresh-image.png')).toBe(true); + }); + + it('does not mark anything when auto-switch is off — nothing is selected to be revealed', async () => { + vi.mocked(selectAutoSwitch).mockReturnValue(false); + autoSwitchedImages.settle(null); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + await handler(buildImageCompleteEvent()); + + expect(autoSwitchedImages.consume('fresh-image.png')).toBe(false); + }); + + it('keeps the dedupe key when only the image half of a mixed result failed', async () => { + // The video lookup succeeded, so its board invalidation and auto-switch already went out; a + // re-delivery re-running them would invalidate twice and move the selection a second time. + vi.mocked(selectAutoSwitch).mockReturnValue(true); + vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); + vi.mocked(getVideoDTOSafe).mockResolvedValueOnce({ + video_name: 'fresh-video.mp4', + video_url: 'mock://fresh-video.mp4', + thumbnail_url: 'mock://thumb/fresh-video.mp4', + is_intermediate: false, + is_starred: false, + board_id: 'board-123', + created_at: '2026-01-01', + updated_at: '2026-01-01', + session_id: 'test-session', + node_id: 'test-node', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const mixed = buildImageCompleteEvent(); + mixed.invocation.type = 'wan_l2v'; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (mixed as any).result = { + image: { image_name: 'lost-image.png' }, + video: { video_name: 'fresh-video.mp4' }, + }; + + await handler(mixed); + expect(getVideoDTOSafe).toHaveBeenCalledTimes(1); + + // The re-delivery must be turned away: the video work already landed. + await handler(mixed); + expect(getVideoDTOSafe).toHaveBeenCalledTimes(1); + expect(getImageDTOSafe).toHaveBeenCalledTimes(1); + }); + + it('redoes only the gallery work on a retry, not the global side effects', async () => { + // The canvas processing flag and $lastProgressEvent are global: by the time a re-delivery + // arrives the user may have started another run, and clearing them again would stop that run's + // spinner and blank its progress. + vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); + + const dispatched: unknown[] = []; + const dispatch = vi.fn((action: unknown) => { + dispatched.push(action); + return { unwrap: () => Promise.resolve(undefined) }; + }); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const canvasEvent = buildImageCompleteEvent(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (canvasEvent as any).origin = 'canvas_workflow_integration'; + + await handler(canvasEvent); + const canvasClears = () => dispatched.filter((a) => (a as { type?: string }).type === 'mock/canvasComplete').length; + expect(canvasClears()).toBe(1); + expect($lastProgressEvent.set).toHaveBeenCalledTimes(1); + + // The retry must redo the lost gallery fetch... + await handler(canvasEvent); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + // ...and nothing else. + expect(canvasClears()).toBe(1); + expect($lastProgressEvent.set).toHaveBeenCalledTimes(1); + }); + + it('does not offer a retry to a delivery whose only output was intermediate', async () => { + // An intermediate image never reaches the gallery, so a lookup failure alongside it leaves + // nothing dispatched — and the retry that could recover the lost image must be allowed. + vi.mocked(getImageDTOSafe) + .mockResolvedValueOnce(null) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockResolvedValueOnce({ image_name: 'intermediate.png', is_intermediate: true, board_id: null } as any); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const twoImages = buildImageCompleteEvent(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (twoImages as any).result = { + image_1: { image_name: 'lost.png' }, + image_2: { image_name: 'intermediate.png' }, + }; + + await handler(twoImages); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + + await handler(twoImages); + expect(getImageDTOSafe).toHaveBeenCalledTimes(4); + }); + it('still processes distinct invocations of the same queue item', async () => { const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); const getState = vi.fn(() => ({})); diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx index a2d03efc1e6..c9c3778cae7 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx @@ -33,6 +33,11 @@ import type { JsonObject } from 'type-fest'; const log = logger('events'); +/** What a completion event still has outstanding. 'done' rejects any re-delivery; + * 'gallery-retryable' means every DTO lookup failed, so a re-delivery may redo the gallery work + * (and only that work — see the handler). */ +type ProcessedInvocationState = 'done' | 'gallery-retryable'; + // These nodes are passthrough nodes. They do not add images/videos to the gallery — their // outputs reference an existing asset — so we must skip the gallery handling for them. // Without 'video' here, a Video Primitive completing mid-run would invalidate the gallery @@ -62,11 +67,12 @@ export const buildOnInvocationComplete = ( // handler runs — so the handler tracks what it has processed itself. The invocation id half of // the key is the prepared node's per-execution UUID, so keys cannot collide across distinct // executions even where item ids restart (in-memory DB); the LRU bounds memory. - const processedInvocations = new LRUCache({ max: 1000 }); + const processedInvocations = new LRUCache({ max: 1000 }); - // Returns how many image DTOs were fetched, so the caller can tell "this event had no gallery - // output" apart from "the output was there but every lookup failed" (see the dedupe-key drop in - // the handler below). + // Returns how many DTOs had gallery work dispatched for them, so the caller can tell "this + // delivery changed nothing" apart from "part of it landed" (see the retry decision in the handler + // below). Paths that bail out before the first dispatch — including an intermediate output, which + // never reaches the gallery — count as nothing dispatched. const addImagesToGallery = async ( data: S['InvocationCompleteEvent'], onLookupFailure: () => void @@ -88,7 +94,7 @@ export const buildOnInvocationComplete = ( for (const imageDTO of imageDTOs) { if (imageDTO.is_intermediate) { - return imageDTOs.length; + return 0; } const board_id = imageDTO.board_id ?? 'none'; @@ -322,7 +328,7 @@ export const buildOnInvocationComplete = ( const nonIntermediate = videoDTOs.filter((v) => !v.is_intermediate); if (nonIntermediate.length === 0) { - return videoDTOs.length; + return 0; } // Force the polymorphic gallery list to refetch so the new video shows up. Note: this is @@ -400,57 +406,64 @@ export const buildOnInvocationComplete = ( log.debug({ data } as JsonObject, `Invocation complete (${data.invocation.type}, ${data.invocation_source_id})`); const invocationKey = `${data.item_id}:${data.invocation.id}`; - if (processedInvocations.has(invocationKey)) { + const processedState = processedInvocations.get(invocationKey); + if (processedState === 'done') { log.trace( { data } as JsonObject, `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` ); return; } + // A re-delivery of an event whose gallery work was lost to a failed lookup redoes that work and + // nothing else. The rest of this handler is not idempotent against a *later* generation: the + // canvas processing flag and $lastProgressEvent are global, so re-running them here would end + // the spinner and blank the progress of whatever is running now. + const isGalleryRetry = processedState === 'gallery-retryable'; // Mark before the awaits below — a duplicate arriving while the DTO fetch is in flight must be - // rejected too. Dropped again below when every DTO lookup failed: the gallery never got any of - // that output, so a re-delivery must be free to redo the work rather than be turned away as a - // duplicate of a delivery that never landed. - processedInvocations.set(invocationKey, true); + // rejected too. + processedInvocations.set(invocationKey, 'done'); let hadLookupFailure = false; const onLookupFailure = () => { hadLookupFailure = true; }; - const nodeExecutionState = $nodeExecutionStates.get()[data.invocation_source_id]; - const updatedNodeExecutionState = getUpdatedNodeExecutionStateOnInvocationComplete( - nodeExecutionState, - data, - completedInvocationKeysByItemId - ); - - if (nodeExecutionState && !updatedNodeExecutionState) { - log.trace( - { data } as JsonObject, - `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` + if (!isGalleryRetry) { + const nodeExecutionState = $nodeExecutionStates.get()[data.invocation_source_id]; + const updatedNodeExecutionState = getUpdatedNodeExecutionStateOnInvocationComplete( + nodeExecutionState, + data, + completedInvocationKeysByItemId ); - } - if (updatedNodeExecutionState) { - upsertExecutionState(updatedNodeExecutionState.nodeId, updatedNodeExecutionState); - } + if (nodeExecutionState && !updatedNodeExecutionState) { + log.trace( + { data } as JsonObject, + `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` + ); + } + + if (updatedNodeExecutionState) { + upsertExecutionState(updatedNodeExecutionState.nodeId, updatedNodeExecutionState); + } - // Clear canvas workflow integration processing state if needed - clearCanvasWorkflowIntegrationProcessing(data); + // Clear canvas workflow integration processing state if needed + clearCanvasWorkflowIntegrationProcessing(data); + } // Add images to gallery (canvas workflow integration results go to staging area automatically) - const fetchedImageCount = await addImagesToGallery(data, onLookupFailure); - const fetchedVideoCount = await addVideosToGallery(data, onLookupFailure); - - $lastProgressEvent.set(null); - - // Drop the dedupe key only when a lookup failed AND nothing was fetched: such a delivery - // dispatched no gallery work at all, so a re-delivery is free to redo everything (the - // node-execution upsert has its own dedupe via completedInvocationKeysByItemId). A partial - // failure keeps the key — the fetched DTOs' board totals and optimistic inserts were already - // dispatched, and a re-delivery re-running them would double-count. - if (hadLookupFailure && fetchedImageCount + fetchedVideoCount === 0) { - processedInvocations.delete(invocationKey); + const dispatchedImageCount = await addImagesToGallery(data, onLookupFailure); + const dispatchedVideoCount = await addVideosToGallery(data, onLookupFailure); + + if (!isGalleryRetry) { + $lastProgressEvent.set(null); + } + + // Leave the event open to a retry only when a lookup failed AND this delivery dispatched no + // gallery work at all: there is then nothing a re-delivery could double up on. A partial + // failure stays 'done' — the DTOs that did resolve had their board totals and optimistic + // inserts dispatched, and re-running those would double-count them. + if (hadLookupFailure && dispatchedImageCount + dispatchedVideoCount === 0) { + processedInvocations.set(invocationKey, 'gallery-retryable'); } }; }; From f8e6dc9b48ef36a819f94ef810cdd88b56146216 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 21:40:52 -0400 Subject: [PATCH 7/8] fix(ui): stop exporting a type nothing imports knip fails the frontend checks on it: SelectedItemRevealDecision is only ever the return type of the function declared beside it. --- .../gallery/components/ImageViewer/selectedItemReveal.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts index 91b7a599ebf..58bb94e4393 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts @@ -10,7 +10,7 @@ * "leave it as it is" answer would strand the reveal on for the rest of the render with no timer * left to end it — the decision must always name the state the overlay ends up in. */ -export type SelectedItemRevealDecision = 'reveal' | 'hide'; +type SelectedItemRevealDecision = 'reveal' | 'hide'; export type SelectedItemRevealInputs = { /** The user's "show progress in viewer" setting. */ From b46cb1b5b48418c21669a0328fcdec3416d35338 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Tue, 18 Aug 2026 23:02:07 -0400 Subject: [PATCH 8/8] fix(ui): repair five defects an adversarial review found in the retry rework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carried across from #9475, which shares this code — the two handlers stay one implementation. - A result can name the same image twice (an image collection concatenates its inputs without deduping). Each occurrence was fetched and counted separately, inflating the board total; and because the retry set is keyed by name, a retry re-admitted the occurrence that had already landed. Outputs are fetched once per distinct name. - An intermediate output returned from the whole pass, abandoning siblings that belong in the gallery. Survivable when the dedupe was event-wide; with per-output tracking those siblings are in nobody's missing set, so nothing could recover them. Intermediates are filtered, as the video path already did. - A retry re-ran the auto-switch, moving the user's selection (and possibly their board) long after they had chosen something else. - A throw inside the gallery work escaped as an unhandled rejection — both call sites discard this handler's promise. - addBoardIdSelectedListener matched galleryViewChanged, which the auto-switch dispatches immediately before imageSelected. The probe it started woke on that very selection and re-selected the first name in a stale list, undoing the auto-switch and revealing the wrong image over the live preview. An explicit selection now cancels the probe. Also pins that consuming one item's marker cannot spend another's. --- .../listeners/boardIdSelected.test.ts | 60 ++++ .../listeners/boardIdSelected.ts | 13 +- .../gallery/store/autoSwitchedImages.test.ts | 10 + .../events/onInvocationComplete.test.ts | 129 ++++++++- .../services/events/onInvocationComplete.tsx | 256 ++++++++++++------ 5 files changed, 369 insertions(+), 99 deletions(-) create mode 100644 invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts new file mode 100644 index 00000000000..f457fac2280 --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts @@ -0,0 +1,60 @@ +import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'; +import type { AppStartListening } from 'app/store/store'; +import { gallerySliceConfig, galleryViewChanged, imageSelected } from 'features/gallery/store/gallerySlice'; +import { api } from 'services/api'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { addBoardIdSelectedListener } from './boardIdSelected'; + +// The listener waits for the board's item list before auto-selecting, so the store needs the API +// slice present (the query is never fulfilled here — the point is what happens meanwhile). +const buildStore = () => { + const listenerMiddleware = createListenerMiddleware(); + addBoardIdSelectedListener(listenerMiddleware.startListening as unknown as AppStartListening); + return configureStore({ + reducer: { + gallery: gallerySliceConfig.slice.reducer, + [api.reducerPath]: api.reducer, + }, + middleware: (getDefaultMiddleware) => + getDefaultMiddleware({ serializableCheck: false }).prepend(listenerMiddleware.middleware), + }); +}; + +describe('addBoardIdSelectedListener', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('does not overwrite a selection made while it was waiting for the board list', async () => { + // The gallery's auto-switch dispatches galleryViewChanged immediately before imageSelected. + // Without the cancel, the probe this starts wakes up on that very selection and re-selects + // from a stale (or empty) list, undoing the auto-switch — and the viewer then reveals the + // wrong item over the live preview, the flash the auto-switch marker exists to prevent. + const store = buildStore(); + + store.dispatch(galleryViewChanged('images')); + store.dispatch(imageSelected('new.png')); + + // Past the probe's 5s give-up, which would otherwise clear the selection outright. + await vi.advanceTimersByTimeAsync(6000); + + expect(store.getState().gallery.selection).toEqual(['new.png']); + }); + + it('still clears the selection when a board switch finds nothing to show', async () => { + // The auto-select probe itself must keep working: a board change with no items selects + // nothing rather than leaving the previous board's item highlighted. + const store = buildStore(); + store.dispatch(imageSelected('from-previous-board.png')); + + store.dispatch(galleryViewChanged('assets')); + await vi.advanceTimersByTimeAsync(6000); + + expect(store.getState().gallery.selection).toEqual([]); + }); +}); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts index 05dd8e9f208..588b39881a8 100644 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts @@ -6,11 +6,22 @@ import { galleryApi } from 'services/api/endpoints/gallery'; export const addBoardIdSelectedListener = (startAppListening: AppStartListening) => { startAppListening({ - matcher: isAnyOf(boardIdSelected, galleryViewChanged), + matcher: isAnyOf(boardIdSelected, galleryViewChanged, imageSelected), effect: async (action, { getState, dispatch, condition, cancelActiveListeners }) => { // Cancel any in-progress instances of this listener, we don't want to select an item from a previous board cancelActiveListeners(); + if (imageSelected.match(action)) { + // An explicit selection settles what should be displayed, so a probe still waiting on a + // board's items must not overwrite it when it resolves. The gallery's auto-switch dispatches + // galleryViewChanged immediately before imageSelected: without this the probe that view + // change starts wakes on the selection that follows it, re-selects the first name in a + // possibly stale cached list, and undoes the switch — and the viewer then reveals that + // wrong image over the live preview, which is the flash the auto-switch marker exists to + // prevent. Cancelling above is the whole effect; there is nothing to auto-select here. + return; + } + if (boardIdSelected.match(action) && action.payload.select) { // This action already has a resource selection - skip the below auto-selection logic return; diff --git a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts index 6e28810a2cd..899397eeaac 100644 --- a/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts +++ b/invokeai/frontend/web/src/features/gallery/store/autoSwitchedImages.test.ts @@ -66,4 +66,14 @@ describe('createAutoSwitchedSelectionMarker', () => { marker.settle('a.png'); expect(marker.consume('a.png')).toBe(false); }); + + it('does not clear a pending marker when a different item is consumed', () => { + // The rendered item can lag a just-auto-switched selection: the viewer consumes for whatever + // is still on screen, and that must not spend the marker the incoming item is owed. + const marker = createAutoSwitchedSelectionMarker(); + marker.record('a.png'); + marker.settle('a.png'); + expect(marker.consume('b.png')).toBe(false); + expect(marker.consume('a.png'), 'the marker survives an unrelated consume').toBe(true); + }); }); diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts index b89ed0ca406..35a2e5c11bb 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.test.ts @@ -379,10 +379,10 @@ describe('onInvocationComplete polymorphic gallery cache', () => { ).toBe(true); }); - it('does not retry a partial failure — the fetched DTOs were already dispatched', async () => { - // One of two image lookups fails: the successful one's board totals and optimistic insert have - // already gone out, so a re-delivery re-running the gallery work would double-count them. The - // dedupe key is only dropped when NOTHING was fetched. + it('retries only the output whose lookup failed, leaving the ones that landed alone', async () => { + // One of two image lookups fails. The successful one's board totals and optimistic insert have + // already gone out, so a re-delivery must not touch it — but the lost one is only recoverable + // here, since nothing re-emits the event on its own. vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); @@ -406,9 +406,79 @@ describe('onInvocationComplete polymorphic gallery cache', () => { await handler(twoImages); expect(getImageDTOSafe).toHaveBeenCalledTimes(2); - // The re-delivery must be treated as a duplicate — no further lookups. await handler(twoImages); + // Exactly one more lookup, and it is the one that failed. + expect(getImageDTOSafe).toHaveBeenCalledTimes(3); + expect(vi.mocked(getImageDTOSafe).mock.calls.at(-1)?.[0]).toBe('first.png'); + + // Once everything has landed, the event is closed again. + await handler(twoImages); + expect(getImageDTOSafe).toHaveBeenCalledTimes(3); + }); + + it('retries a failed lookup inside an image collection', async () => { + // Collections are where partial failure is actually plausible: the user sees most of a batch + // and silently misses one image, its auto-switch, and its board count. + vi.mocked(getImageDTOSafe).mockResolvedValueOnce(null); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const collection = buildImageCompleteEvent(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (collection as any).result = { + collection: [{ image_name: 'batch-1.png' }, { image_name: 'batch-2.png' }, { image_name: 'batch-3.png' }], + }; + + await handler(collection); + expect(getImageDTOSafe).toHaveBeenCalledTimes(3); + + await handler(collection); + expect(getImageDTOSafe).toHaveBeenCalledTimes(4); + expect(vi.mocked(getImageDTOSafe).mock.calls.at(-1)?.[0]).toBe('batch-1.png'); + }); + + it('lets a duplicate that overlapped a lost delivery become the retry', async () => { + // The duplicate arrives while the first delivery is still fetching, so it cannot be told yet + // that the fetch will fail. Rejecting it outright strands the output: there is no third event. + let resolveFirstLookup: (value: null) => void = () => {}; + vi.mocked(getImageDTOSafe).mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirstLookup = resolve; + }) + ); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const event = buildImageCompleteEvent(); + const first = handler(event); + const duplicate = handler(event); + + // The first delivery's only lookup fails, losing the image. + resolveFirstLookup(null); + await Promise.all([first, duplicate]); + + // The duplicate picked the work back up rather than being discarded. expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + expect(vi.mocked(getImageDTOSafe).mock.calls.at(-1)?.[0]).toBe('fresh-image.png'); }); it('marks the selection it auto-switches to, so the viewer does not reveal it as a user click', async () => { @@ -455,7 +525,7 @@ describe('onInvocationComplete polymorphic gallery cache', () => { expect(autoSwitchedImages.consume('fresh-image.png')).toBe(false); }); - it('keeps the dedupe key when only the image half of a mixed result failed', async () => { + it('retries only the image half of a mixed result, not the video that landed', async () => { // The video lookup succeeded, so its board invalidation and auto-switch already went out; a // re-delivery re-running them would invalidate twice and move the selection a second time. vi.mocked(selectAutoSwitch).mockReturnValue(true); @@ -495,11 +565,12 @@ describe('onInvocationComplete polymorphic gallery cache', () => { await handler(mixed); expect(getVideoDTOSafe).toHaveBeenCalledTimes(1); + expect(getImageDTOSafe).toHaveBeenCalledTimes(1); - // The re-delivery must be turned away: the video work already landed. + // The re-delivery refetches the lost image and leaves the video alone. await handler(mixed); + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); expect(getVideoDTOSafe).toHaveBeenCalledTimes(1); - expect(getImageDTOSafe).toHaveBeenCalledTimes(1); }); it('redoes only the gallery work on a retry, not the global side effects', async () => { @@ -540,9 +611,9 @@ describe('onInvocationComplete polymorphic gallery cache', () => { expect($lastProgressEvent.set).toHaveBeenCalledTimes(1); }); - it('does not offer a retry to a delivery whose only output was intermediate', async () => { - // An intermediate image never reaches the gallery, so a lookup failure alongside it leaves - // nothing dispatched — and the retry that could recover the lost image must be allowed. + it('retries the lost output alongside an intermediate one, without redoing the intermediate', async () => { + // An intermediate image never reaches the gallery, so there is nothing to redo for it — but the + // lookup that failed beside it must still be recoverable. vi.mocked(getImageDTOSafe) .mockResolvedValueOnce(null) // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -569,8 +640,42 @@ describe('onInvocationComplete polymorphic gallery cache', () => { await handler(twoImages); expect(getImageDTOSafe).toHaveBeenCalledTimes(2); + // Only the lost output is refetched — the intermediate one never had gallery work to redo. await handler(twoImages); - expect(getImageDTOSafe).toHaveBeenCalledTimes(4); + expect(getImageDTOSafe).toHaveBeenCalledTimes(3); + expect(vi.mocked(getImageDTOSafe).mock.calls.at(-1)?.[0]).toBe('lost.png'); + }); + + it('serializes several duplicates waiting on one lost delivery into a single retry', async () => { + // All three duplicates are parked on the same in-flight delivery. When it fails they all wake + // up; only one may pick the work back up, or the retried output lands two or three times over. + let resolveFirstLookup: (value: null) => void = () => {}; + vi.mocked(getImageDTOSafe).mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirstLookup = resolve; + }) + ); + + const dispatch = vi.fn(() => ({ unwrap: () => Promise.resolve(undefined) })); + const getState = vi.fn(() => ({})); + + const handler = buildOnInvocationComplete( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getState as any, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + dispatch as any, + new Map() + ); + + const event = buildImageCompleteEvent(); + const deliveries = [handler(event), handler(event), handler(event), handler(event)]; + + resolveFirstLookup(null); + await Promise.all(deliveries); + + // One failed lookup plus exactly one retry. + expect(getImageDTOSafe).toHaveBeenCalledTimes(2); }); it('still processes distinct invocations of the same queue item', async () => { diff --git a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx index c9c3778cae7..d5fe150d645 100644 --- a/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx +++ b/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx @@ -33,10 +33,16 @@ import type { JsonObject } from 'type-fest'; const log = logger('events'); -/** What a completion event still has outstanding. 'done' rejects any re-delivery; - * 'gallery-retryable' means every DTO lookup failed, so a re-delivery may redo the gallery work - * (and only that work — see the handler). */ -type ProcessedInvocationState = 'done' | 'gallery-retryable'; +/** + * What a completion event still has outstanding. + * + * `done` rejects any re-delivery. `retryable` names the outputs whose DTO lookup failed — the ones + * that never reached the gallery — so a re-delivery can fetch exactly those again. Tracking the + * missing outputs rather than the event as a whole is what makes a partial failure recoverable: an + * event-wide flag would either re-run the outputs that did land (double-counting their board totals + * and optimistic inserts) or abandon the one that did not. + */ +type ProcessedInvocationState = { status: 'done' } | { status: 'retryable'; missingNames: ReadonlySet }; // These nodes are passthrough nodes. They do not add images/videos to the gallery — their // outputs reference an existing asset — so we must skip the gallery handling for them. @@ -68,23 +74,34 @@ export const buildOnInvocationComplete = ( // the key is the prepared node's per-execution UUID, so keys cannot collide across distinct // executions even where item ids restart (in-memory DB); the LRU bounds memory. const processedInvocations = new LRUCache({ max: 1000 }); + // Deliveries currently fetching, so a duplicate can wait for one instead of being discarded. + // Entries live only for the duration of a pass; the LRU above is what bounds long-term memory. + const inFlightDeliveries = new Map>(); - // Returns how many DTOs had gallery work dispatched for them, so the caller can tell "this - // delivery changed nothing" apart from "part of it landed" (see the retry decision in the handler - // below). Paths that bail out before the first dispatch — including an intermediate output, which - // never reaches the gallery — count as nothing dispatched. + // `retryNames`, when set, restricts this pass to the outputs a previous delivery lost to a failed + // lookup — everything else already landed and must not be dispatched twice. const addImagesToGallery = async ( data: S['InvocationCompleteEvent'], - onLookupFailure: () => void - ): Promise => { + onLookupFailure: (imageName: string) => void, + retryNames: ReadonlySet | null + ) => { + // A retry exists to land an output the gallery lost, not to re-run the handoff around it. By + // the time a re-delivery arrives the user has had time to select something else, and pulling + // the selection back would be a worse failure than the one being repaired. + const isRetry = retryNames !== null; if (nodeTypeDenylist.includes(data.invocation.type)) { log.trace(`Skipping denylisted node type (${data.invocation.type})`); - return 0; + return; } - const imageDTOs = await getResultImageDTOs(data, onLookupFailure); + const fetchedImageDTOs = await getResultImageDTOs(data, onLookupFailure, retryNames); + // Intermediates never reach the gallery. Dropping them here rather than bailing out of the + // whole pass on the first one (which is what this used to do) matters now that a delivery's + // outputs are tracked individually: a sibling abandoned that way is in nobody's missing set, + // so no re-delivery could ever recover it. Mirrors addVideosToGallery. + const imageDTOs = fetchedImageDTOs.filter((imageDTO) => !imageDTO.is_intermediate); if (imageDTOs.length === 0) { - return 0; + return; } // For efficiency's sake, we want to minimize the number of dispatches and invalidations we do. @@ -93,10 +110,6 @@ export const buildOnInvocationComplete = ( const getImageNamesArg = selectGetImageNamesQueryArgs(getState()); for (const imageDTO of imageDTOs) { - if (imageDTO.is_intermediate) { - return 0; - } - const board_id = imageDTO.board_id ?? 'none'; // update the total images for the board boardTotalAdditions[board_id] = (boardTotalAdditions[board_id] || 0) + 1; @@ -205,15 +218,15 @@ export const buildOnInvocationComplete = ( const autoSwitch = selectAutoSwitch(getState()); - if (!autoSwitch) { - return imageDTOs.length; + if (!autoSwitch || isRetry) { + return; } // Finally, we may need to autoswitch to the new image. We'll only do it for the last image in the list. const lastImageDTO = imageDTOs.at(-1); if (!lastImageDTO) { - return imageDTOs.length; + return; } const { image_name } = lastImageDTO; @@ -249,36 +262,44 @@ export const buildOnInvocationComplete = ( // Select the image immediately since we've optimistically updated the cache dispatch(imageSelected(lastImageDTO.image_name)); } - - return imageDTOs.length; }; // getImageDTOSafe swallows fetch errors and returns null, which downstream is indistinguishable - // from "this node produced no image". onLookupFailure separates the two: the handler drops this - // event's dedupe key when it fires, so a re-delivery redoes the gallery work instead of being - // turned away as a duplicate of a delivery whose output never reached the gallery. + // from "this node produced no image". onLookupFailure separates the two, naming the output that + // was lost so a re-delivery can fetch just that one again. const getResultImageDTOs = async ( data: S['InvocationCompleteEvent'], - onLookupFailure: () => void + onLookupFailure: (imageName: string) => void, + retryNames: ReadonlySet | null ): Promise => { const { result } = data; const imageDTOs: ImageDTO[] = []; + const fetched = new Set(); + const fetch = async (imageName: string) => { + if (retryNames !== null && !retryNames.has(imageName)) { + return; + } + // A result can name the same image twice (an image collection concatenates its inputs + // without deduping). It is still one image: fetching it once keeps its board total counted + // once, and keeps the retry set — which is keyed by name — from re-admitting an occurrence + // that already landed. + if (fetched.has(imageName)) { + return; + } + fetched.add(imageName); + const imageDTO = await getImageDTOSafe(imageName); + if (imageDTO) { + imageDTOs.push(imageDTO); + } else { + onLookupFailure(imageName); + } + }; for (const [_name, value] of objectEntries(result)) { if (isImageField(value)) { - const imageDTO = await getImageDTOSafe(value.image_name); - if (imageDTO) { - imageDTOs.push(imageDTO); - } else { - onLookupFailure(); - } + await fetch(value.image_name); } else if (isImageFieldCollection(value)) { for (const imageField of value) { - const imageDTO = await getImageDTOSafe(imageField.image_name); - if (imageDTO) { - imageDTOs.push(imageDTO); - } else { - onLookupFailure(); - } + await fetch(imageField.image_name); } } } @@ -287,17 +308,26 @@ export const buildOnInvocationComplete = ( const getResultVideoDTOs = async ( data: S['InvocationCompleteEvent'], - onLookupFailure: () => void + onLookupFailure: (videoName: string) => void, + retryNames: ReadonlySet | null ): Promise => { const { result } = data; const videoDTOs: VideoDTO[] = []; + const fetched = new Set(); for (const [_name, value] of objectEntries(result)) { if (isVideoField(value)) { + if (retryNames !== null && !retryNames.has(value.video_name)) { + continue; + } + if (fetched.has(value.video_name)) { + continue; + } + fetched.add(value.video_name); const videoDTO = await getVideoDTOSafe(value.video_name); if (videoDTO) { videoDTOs.push(videoDTO); } else { - onLookupFailure(); + onLookupFailure(value.video_name); } } } @@ -315,20 +345,23 @@ export const buildOnInvocationComplete = ( // so the stuck "Saving video" spinner goes away on its own. const addVideosToGallery = async ( data: S['InvocationCompleteEvent'], - onLookupFailure: () => void - ): Promise => { + onLookupFailure: (videoName: string) => void, + retryNames: ReadonlySet | null + ) => { + // See addImagesToGallery: a retry does not redo the auto-switch. + const isRetry = retryNames !== null; if (nodeTypeDenylist.includes(data.invocation.type)) { - return 0; + return; } - const videoDTOs = await getResultVideoDTOs(data, onLookupFailure); + const videoDTOs = await getResultVideoDTOs(data, onLookupFailure, retryNames); if (videoDTOs.length === 0) { - return 0; + return; } const nonIntermediate = videoDTOs.filter((v) => !v.is_intermediate); if (nonIntermediate.length === 0) { - return 0; + return; } // Force the polymorphic gallery list to refetch so the new video shows up. Note: this is @@ -346,13 +379,13 @@ export const buildOnInvocationComplete = ( dispatch(galleryApi.util.invalidateTags(getTagsToInvalidateForBoardAffectingMutation(affectedBoards))); const autoSwitch = selectAutoSwitch(getState()); - if (!autoSwitch) { - return videoDTOs.length; + if (!autoSwitch || isRetry) { + return; } const lastVideoDTO = nonIntermediate.at(-1); if (!lastVideoDTO) { - return videoDTOs.length; + return; } const { video_name } = lastVideoDTO; @@ -377,8 +410,6 @@ export const buildOnInvocationComplete = ( } dispatch(imageSelected(video_name)); } - - return videoDTOs.length; }; const clearCanvasWorkflowIntegrationProcessing = (data: S['InvocationCompleteEvent']) => { @@ -402,32 +433,29 @@ export const buildOnInvocationComplete = ( } }; - return async (data: S['InvocationCompleteEvent']) => { - log.debug({ data } as JsonObject, `Invocation complete (${data.invocation.type}, ${data.invocation_source_id})`); - - const invocationKey = `${data.item_id}:${data.invocation.id}`; - const processedState = processedInvocations.get(invocationKey); - if (processedState === 'done') { - log.trace( - { data } as JsonObject, - `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` - ); - return; - } - // A re-delivery of an event whose gallery work was lost to a failed lookup redoes that work and - // nothing else. The rest of this handler is not idempotent against a *later* generation: the - // canvas processing flag and $lastProgressEvent are global, so re-running them here would end - // the spinner and blank the progress of whatever is running now. - const isGalleryRetry = processedState === 'gallery-retryable'; - // Mark before the awaits below — a duplicate arriving while the DTO fetch is in flight must be - // rejected too. - processedInvocations.set(invocationKey, 'done'); - let hadLookupFailure = false; - const onLookupFailure = () => { - hadLookupFailure = true; + /** + * One pass over a completion event. `retryNames` is null for a first delivery and otherwise names + * the outputs an earlier delivery lost, restricting this pass to those. + * + * Returns the outputs whose lookup failed this time. Bookkeeping is done here, before the + * returned promise settles, so a duplicate waiting on it always observes the final state. + */ + const deliver = async ( + data: S['InvocationCompleteEvent'], + invocationKey: string, + retryNames: ReadonlySet | null + ): Promise => { + const isRetry = retryNames !== null; + const missingNames = new Set(); + const onLookupFailure = (itemName: string) => { + missingNames.add(itemName); }; - if (!isGalleryRetry) { + // A retry redoes the lost gallery work and nothing else: the rest of this handler is not + // idempotent against a *later* generation, because the canvas processing flag and + // $lastProgressEvent are global. Re-running them would end the spinner and blank the progress + // of whatever is running by then. + if (!isRetry) { const nodeExecutionState = $nodeExecutionStates.get()[data.invocation_source_id]; const updatedNodeExecutionState = getUpdatedNodeExecutionStateOnInvocationComplete( nodeExecutionState, @@ -450,20 +478,76 @@ export const buildOnInvocationComplete = ( clearCanvasWorkflowIntegrationProcessing(data); } - // Add images to gallery (canvas workflow integration results go to staging area automatically) - const dispatchedImageCount = await addImagesToGallery(data, onLookupFailure); - const dispatchedVideoCount = await addVideosToGallery(data, onLookupFailure); + try { + // Add images to gallery (canvas workflow integration results go to staging area automatically) + await addImagesToGallery(data, onLookupFailure, retryNames); + await addVideosToGallery(data, onLookupFailure, retryNames); + + if (!isRetry) { + $lastProgressEvent.set(null); + } + } catch (error) { + // Both call sites discard this handler's promise, so a throw here would surface as an + // unhandled rejection and nothing else. Log it and let the bookkeeping below run: the + // outputs whose lookups failed are still worth recording as retryable. + log.error({ data, error } as JsonObject, `Error handling invocation complete: ${String(error)}`); + } finally { + if (missingNames.size > 0) { + processedInvocations.set(invocationKey, { status: 'retryable', missingNames }); + } + } + }; + + return async (data: S['InvocationCompleteEvent']) => { + log.debug({ data } as JsonObject, `Invocation complete (${data.invocation.type}, ${data.invocation_source_id})`); + + const invocationKey = `${data.item_id}:${data.invocation.id}`; + const logDuplicate = () => { + log.trace( + { data } as JsonObject, + `Ignoring duplicate invocation complete (${data.invocation.type}, ${data.invocation_source_id})` + ); + }; - if (!isGalleryRetry) { - $lastProgressEvent.set(null); + // A duplicate that lands while the first delivery is still fetching waits for it rather than + // being discarded: if that delivery lost outputs to failed lookups, this duplicate is the only + // thing that can recover them — nothing re-emits the event on its own. This check must come + // before the 'done' one, which the in-flight delivery has already written by now. + // + // Several duplicates can be waiting here at once. They resume one at a time, and the first to + // find work marks the event 'done' again before it suspends, so the rest fall through to the + // duplicate branch rather than starting parallel retries. + const inFlight = inFlightDeliveries.get(invocationKey); + if (inFlight) { + await inFlight; + if (processedInvocations.get(invocationKey)?.status !== 'retryable') { + logDuplicate(); + return; + } + } else if (processedInvocations.get(invocationKey)?.status === 'done') { + logDuplicate(); + return; } - // Leave the event open to a retry only when a lookup failed AND this delivery dispatched no - // gallery work at all: there is then nothing a re-delivery could double up on. A partial - // failure stays 'done' — the DTOs that did resolve had their board totals and optimistic - // inserts dispatched, and re-running those would double-count them. - if (hadLookupFailure && dispatchedImageCount + dispatchedVideoCount === 0) { - processedInvocations.set(invocationKey, 'gallery-retryable'); + const state = processedInvocations.get(invocationKey); + const retryNames = state?.status === 'retryable' ? state.missingNames : null; + // Mark before the awaits below so a duplicate cannot start a second pass over the same outputs + // while this one is in flight; the handshake above is what lets it retry afterwards instead. + processedInvocations.set(invocationKey, { status: 'done' }); + + const delivery = deliver(data, invocationKey, retryNames); + // Waiters only need to know when the pass finished, not whether it threw. + const settled = delivery.then( + () => undefined, + () => undefined + ); + inFlightDeliveries.set(invocationKey, settled); + try { + await delivery; + } finally { + if (inFlightDeliveries.get(invocationKey) === settled) { + inFlightDeliveries.delete(invocationKey); + } } }; };