fix(ui): don't strand the viewer under the video progress overlay - #149
Open
lstein wants to merge 2 commits into
Open
fix(ui): don't strand the viewer under the video progress overlay#149lstein wants to merge 2 commits into
lstein wants to merge 2 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
During a video render, the viewer was effectively locked. The progress-preview overlay is opaque and sits on top of the selected media; gallery thumbnail clicks changed the selection underneath, but nothing visibly happened until a tab switch remounted the viewer (which resets the context's nanostore atoms — that's why the tab-switch "workaround" worked). 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 (invoke-ai#9217).CurrentVideoPreviewshowed the overlay unconditionally whenever a progress image existed — with a gallery full of videos, every click during a multi-minute render was silently swallowed. 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 — the fresh-context adversarial review caught that the mount run (whereimageToRenderis still null) would otherwise erase the "previous item was a video" fact and kill the video→image reveal, which is half the point of sharing the ref.3. The post-render "resolve" could strand the overlay
After completion (auto-switch on), the overlay is intentionally held until the final media loads, to create the progress-resolves-into-result illusion. That clear only fired 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 permanently. 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; the review verified it cannot fire on top of a live preview.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.Adversarial review
A fresh-context review attacked the atom lifecycle (unmount-order interleavings on component swaps, cross-component stale timers, stuck-ON reveal), the shared ref (preload lag, rapid A→B→A clicks, deselect paths), the failsafe (multi-GPU completion sequences, coexisting timers, firing over a live preview), and the error-handler clear. One HIGH finding (the mount-run ref overwrite in item 2) and one LOW (play button exposure during reveal, item 1's
!isPlayingguard) — both fixed. Remaining known race: the auto-switch reveal race the image path already has (fixed upstream-side by the identity-based mechanism onfix/viewer-progress-image-handoff; can be ported to videos when that merges — the two changes compose).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.🤖 Generated with Claude Code