fix(ui): don't strand the viewer under the video progress overlay - #9475
fix(ui): don't strand the viewer under the video progress overlay#9475lstein wants to merge 5 commits into
Conversation
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 invoke-ai#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 <video> never fires it - stranding the overlay permanently. The video error handler now clears a pending resolve, and a 10 s failsafe in the context drops the illusion rather than strand the overlay. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…GPU) CurrentImagePreview tiles per-session previews when more than one render runs concurrently; CurrentVideoPreview only ever rendered the single shared latest preview, so parallel sessions overwrote each other's frames in place. Port the ProgressImageTiles branch, mirroring the image viewer exactly ($activeProgressData is already tracked per-session in the shared context). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JPPhoto
left a comment
There was a problem hiding this comment.
A few things to fix:
-
invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx:115treats any video-name change as a user reveal. An asynchronously auto-selected completed video can therefore hide a newer render's progress preview for 2 seconds.#9434fixes this for images withautoSwitchedImages, but PR 9475 defers the video equivalent. Test: enable auto-switch, finish video B, start video C, let C emit progress, then let B's selection arrive; the progress overlay must remain visible. -
invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx:187and:216clear global progress state without checking active sessions. If session A finishes while session B remains active, A's preview is removed from$progressData, but the global image/event can be cleared by the failsafe, metadata load, error path, or cancellation. B's tile then disappears until another B progress event. Test: emit progress for A and B, finish A, leave B active without another event, then trigger the 10-second failsafe or A's clear path; B's preview must remain visible.
Some alternative paths worth considering:
-
Instead of the current shared
lastRenderedItemNameRef,isTemporarilyShowingSelectedImage, and global resolve timer: use an item-owned reducer/state machine. Track each item asrunning,resolving,canceled, orfailed, with item-specific media readiness, reveal source, and timeout. -
Instead of the current implicit combination of progress state, completion handoff, and selection reveal: use three explicit state machines. Keep queue progress, completed-item handoff, and selected-media reveal separate, with events carrying
item_idand source. -
Instead of patching only the shared clear path: add
progressOwnerItemIdandresolveItemId, makeonLoadImageitem-specific, and derive the displayed fallback from$activeProgressDatawhen the current global owner finishes. Also extend the#9434auto-switch registry to videos.
Summary
During a video render, the viewer is effectively locked. The progress-preview overlay is opaque and sits on top of the selected media; gallery thumbnail clicks change the selection underneath, but nothing visibly happens until a tab switch remounts the viewer panel (which resets the
ImageViewerContextatoms — that's why switching tabs "fixes" it). Found while chaining Wan i2v renders with a gallery full of videos: every click during a multi-minute render is silently swallowed. Three causes, fixed together:1. Videos never got the temporary reveal
CurrentImagePreviewlifts the overlay for 2 s when the user clicks a thumbnail mid-render (#9217).CurrentVideoPreviewshows the overlay unconditionally whenever a progress image exists. The reveal is now ported: the clicked video appears (first frame + play button) for 2 s, then the live preview returns. An actively-playing video is never re-covered — an explicit play is a stronger signal than the click that revealed it, and re-covering would leave audio running under an opaque overlay with unreachable controls. The overlay returns when the player is closed.2. Media-type switches reset the reveal's memory
The reveal fires on a change of rendered item. That previous-item tracking was a per-component ref, but image↔video clicks swap the mounted preview component, so the ref reset and the first reveal after every type switch was swallowed. The ref now lives in the shared
ImageViewerContext. The image side deliberately does not overwrite it while a selection's preload is still pending — an adversarial review of this change caught that the mount run (whereimageToRenderis still null) would otherwise erase the "previous item was a video" fact and kill the video→image reveal.3. The post-render "resolve" can strand the overlay
After completion (auto-switch on), the overlay is intentionally held until the final media loads, creating the progress-resolves-into-result illusion. That clear only fires from the final image's
onLoad/ final video'sonLoadedMetadata. On a slow connection the load lags far behind completion, and an errored<video>(transient 401, network failure) never fires it — stranding the overlay until the user switches tabs or reloads. Now: the video error handler clears a pending resolve (ref-gated, so it cannot blank a live mid-render preview), and a 10 s failsafe in the context drops the illusion rather than strand the overlay. The failsafe is defused by the load callback, by any new render's progress event, and by provider unmount.4. Multi-GPU: concurrent sessions overwrote each other's video preview
CurrentImagePreviewtiles per-session previews when more than one render runs concurrently (ProgressImageTiles); the video overlay only ever rendered the single shared latest preview, so parallel sessions overwrote each other's frames in place. The tiles branch is now ported, mirroring the image viewer exactly —$activeProgressDatawas already tracked per-session in the shared context.Review
A fresh-context adversarial review attacked the atom lifecycle (unmount-order interleavings on preview-component swaps, cross-component stale timers, stuck-ON reveal), the shared ref (preload lag, rapid A→B→A clicks, deselect paths), the failsafe (sequential multi-GPU completions, coexisting timers, firing over a live preview), and the error-handler clear. Two findings, both fixed in this diff (the mount-run ref overwrite in item 2 and the
!isPlayingoverlay guard in item 1); the rest of the attack log came back clean.Relation to #9434
This overlaps textually with #9434 (viewer progress-image handoff) in
context.tsxandCurrentImagePreview.tsx— whichever merges second I'll rebase and resolve. They compose: #9434's identity-based auto-switch suppression (autoSwitchedImages) closes a reveal race this PR inherits for videos in the same form the image path already has (an auto-switch racing a quickly-started next render can read as a user click and flash a 2 s reveal). Once both are in, the video reveal can consume the same registry — happy to do that as the follow-up on whichever lands last.Testing
pnpm lint:tsc, eslint, prettier clean;CurrentVideoPreview.test.tsextended with assertions pinning the overlay's reveal/playing guards, the shared-ref wiring, and the error-path clear. A manual pass on a local build (mid-render clicks in both type directions, playback during reveal, slow-connection completion) is queued on my side — I'll report results on this PR before it's ready for merge.🤖 Generated with Claude Code