From eebcefbcf0e5ba0cfa4142f23fcbb10f07c9347f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 1 Aug 2026 18:29:21 -0400 Subject: [PATCH 01/27] 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 02/27] 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 54c76c38c8d20e4dfe467c1ce53b2ff1325caf23 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 6 Aug 2026 18:39:24 -0400 Subject: [PATCH 03/27] fix(ui): don't strand the viewer under the video progress overlay During a video render, the progress-preview overlay swallowed every gallery thumbnail click: the selection changed underneath, but the opaque overlay stayed on top, so nothing visibly happened until a tab switch remounted the viewer. Three causes, three fixes: - CurrentVideoPreview never implemented the temporary reveal that CurrentImagePreview got in #9217. Port it: clicking a thumbnail mid-render now lifts the overlay for 2 s so the click visibly lands, then the live preview returns. An actively-playing video is never re-covered (audio would keep running under an opaque overlay with unreachable controls); the overlay returns when the player closes. - The reveal's previous-item tracking was per-component, so any click that switched media type (image <-> video swaps the mounted preview component) reset it and the reveal was swallowed. The ref now lives in the shared ImageViewerContext; the image side is careful not to null it while a preload is still pending (adversarial-review finding: the mount run would otherwise erase the previous-video fact and kill the video->image reveal). - After completion, the "preview resolves into the final media" clear only fired from the final media's load callback. On a slow connection that lags far behind completion, and an errored