Skip to content

fix(ui): don't strand the viewer under the progress overlay, for images or videos - #9475

Merged
lstein merged 47 commits into
invoke-ai:mainfrom
lstein:fix/viewer-video-progress-overlay
Aug 22, 2026
Merged

fix(ui): don't strand the viewer under the progress overlay, for images or videos#9475
lstein merged 47 commits into
invoke-ai:mainfrom
lstein:fix/viewer-video-progress-overlay

Conversation

@lstein

@lstein lstein commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

During a render, the viewer is effectively locked. The progress-preview overlay is opaque and
sits on top of the selected media; gallery clicks change the selection underneath, but nothing
visibly happens until a tab switch remounts the viewer panel. Found while chaining Wan i2v renders
with a gallery full of videos: every click during a multi-minute render is silently swallowed.

Note

This PR now also contains #9434 (viewer progress-image handoff), which is closed in favour of
this one. The two had converged into a single change maintained in two branches — three commits
existed in both with identical subjects, this branch's auto-switch marker and completion dedupe
were ported byte-identical from #9434, and #9434 had since adopted this branch's extracted reveal
controller. They conflicted in four files in either merge order, so the split was costing a
resolution per review round with a standing risk of the two copies drifting. Sections 5–7 below
are #9434's; its review history is on that PR.

1. Videos never got the temporary reveal

CurrentImagePreview lifts the overlay for 2 s when the user clicks a thumbnail mid-render
(#9217). CurrentVideoPreview showed the overlay unconditionally whenever a progress image
existed. 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.

3. An auto-switch to a finished item read as a user click

The reveal fires on any change of the selected item, so an auto-switch to a just-finished render
hid the next render's live preview for 2 s. The auto-switch selection is dispatched only after
onInvocationComplete's DTO fetch, so a quickly-started next render's first progress event can
land ahead of it and reset $isProgressImageResolving — timing cannot distinguish the handoff from
a click. Fixed by identity: the gallery records the name it is auto-switching to and the reveal
consumes it on that item's first render. The marker is scoped to the selection it was recorded
for, settled by a redux listener matching on state change rather than action type, so an
auto-switch that never renders is dropped the moment the selection moves on and can never swallow
a genuine later click on the same item.

4. Multi-GPU: concurrent sessions overwrote each other's video preview

CurrentImagePreview tiles per-session previews when more than one render runs concurrently; 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.

5. The reveal was slow (from #9434)

CurrentImagePreview gated rendering behind an off-DOM preload of the full-resolution PNG (often
several MB), and only that onLoad cleared the overlay — so the stale latent preview stayed on
screen for the entire download. A 256px WEBP thumbnail already exists for every image and is
typically higher resolution than the latent preview it replaces; the reveal is now gated on it,
and DndImage swaps the full image in, in place, once it finishes.

The preload also used the raw imageDTO.image_url while DndImage requests
useMediaUrl(imageDTO.image_url), which appends a media-cookie version — a different key, so the
same bytes were fetched twice (measured: 2 requests when the URLs differ, 1 when they match). The
reuse here is the document's list of available images, keyed by URL and not the HTTP cache, so it
still holds in multiuser mode where images are served Cache-Control: private, no-store.

6. A failed preload wedged the overlay (from #9434)

The preload now settles on success or error and reports through the lifecycle's identity-gated
onLoadImage(sessionId), so a failed load cannot wedge the overlay and a late-settling thumbnail
from an earlier session cannot cut a different session's resolve illusion short.

7. Duplicate invocation_complete deliveries re-ran the gallery work (from #9434)

A duplicate completion double-counted the optimistic board totals and re-dispatched the auto-switch.
The handler now tracks processed invocations itself and returns before any gallery work on a
duplicate, marking the key before the DTO-fetch await so a duplicate landing mid-flight is rejected
too. The shared completedInvocationKeysByItemId map could not be used: the workflow coordinator
pre-marks first-delivery events for non-active workflow items, so keying the early return off it
would have skipped gallery work for legitimate queued workflow completions.

How the merge was resolved

Four files conflicted; all four resolved toward this branch's reveal controller.

#9434 carried a stateless getSelectedItemRevealDecision() whose caller managed the previous-item
ref, the auto-switch marker and the timer by hand. createSelectedItemRevealController owns all of
that and adds sequencing the decision function had no way to express: the resolve-window deferral
(a click landing inside the window keeps its identity until the window ends, so it can still be
revealed), the SELECTION_CLEARED sentinel (re-selecting the item that was just cleared is a
click, and must reveal), and the StrictMode re-arm. Those are the two "inherited holes" an earlier
revision of this PR listed as open — they are fixed here, not deferred.

Everything else #9434 contributed to the conflicted files is kept: the thumbnail-gated preload, the
error-path overlay clear, and its onInvocationComplete coverage. CurrentImagePreview's wiring
test is rewritten against the controller; of its four assertions, one is now covered by a real unit
test on the controller (the marker is consumed on every rendered-item change even with no progress
showing) and one became the routing check CurrentVideoPreview already carries. The only test
dropped without a counterpart asserted that the decision function returned nothing but 'reveal'
or 'hide' — a statement about an API that no longer exists.

Every behavior that had a test on either branch still has one: the merged tree's test inventory is a
superset of both sides apart from those ten titles, nine of which were checked individually against
their counterparts.

One rule did not survive, deliberately. The controller's resolve-window deferral preserves a
single deferred identity, so two changes of the rendered item entirely inside one resolve window
that end back on the item showing when the window opened read as "nothing changed" and do not
reveal; #9434's function would have revealed. Its answer was right for the wrong reason — the ref
had advanced only because that version overwrote it unconditionally, which is the bug that lost the
first click in the far more common single-change case. The window is bounded by
RESOLVE_TIMEOUT_MS (3 s) and the sequence needs an intervening null render, so the controller's
behavior is kept.

One coverage hole the merge opened, now closed. The rewritten wiring test dropped #9434's only
assertion that the component writes $isTemporarilyShowingSelectedImage, and nothing else covers
it: the controller's tests substitute their own setRevealed, CurrentVideoPreview's assertions
are all on the read side, and neither component is ever mounted. Replacing setRevealed with a
no-op in both previews left all 26 wiring assertions — and the full suite — green with the reveal
completely dead. Both wiring tests now fail against that mutation.

Review

Fresh-context adversarial reviews 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 auto-switch registry (unconsumed entries,
wrong-render consumption, TTL/bound edges), the error-handler clear, and — separately — the merge
resolution itself, hunting for a fix that vanished with the side it came from.

Stacked on this

#9520 refactors the reveal into an item-owned state machine on top of this branch. It is
review-ready but should land after this one; its diff is this PR plus ~660 lines of restructuring.

Testing

pnpm lint (tsc, eslint, prettier, dpdm) and the full vitest suite are clean on the merged tree.

lstein and others added 4 commits August 3, 2026 11:38
…king

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) <noreply@anthropic.com>
…review

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 (invoke-ai#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 <noreply@anthropic.com>
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>
@github-actions github-actions Bot added the frontend PRs that change frontend files label Aug 6, 2026
@lstein lstein added the 6.14.0 label Aug 6, 2026
@lstein lstein moved this to 6.14.x Theme: USER EXPERIENCE in Invoke - Community Roadmap Aug 6, 2026
lstein and others added 5 commits August 6, 2026 20:08
…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 JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to fix:

  • invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx:115 treats 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. #9434 fixes this for images with autoSwitchedImages, 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:187 and :216 clear 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 as running, resolving, canceled, or failed, 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_id and source.

  • Instead of patching only the shared clear path: add progressOwnerItemId and resolveItemId, make onLoadImage item-specific, and derive the displayed fallback from $activeProgressData when the current global owner finishes. Also extend the #9434 auto-switch registry to videos.

lstein and others added 2 commits August 20, 2026 08:14
…verlay fix

The two PRs had become one change maintained in two branches: three commits existed in both
with identical subjects, 9475's marker and completion dedupe were ported byte-identical from
9434, and 9434 had since taken 9475's extracted reveal controller. They conflicted in four
files in either merge order, so the split was costing a resolution per round with a standing
risk of the two copies drifting.

The reveal is resolved to 9475's controller throughout. 9434 carried a stateless
getSelectedItemRevealDecision() whose caller managed the previous-item ref, the auto-switch
marker and the timer by hand; the controller owns all of it and adds the sequencing 9434's
version had no way to express -- resolve-window deferral, the SELECTION_CLEARED sentinel, and
the StrictMode re-arm. Every branch the decision function encoded has a counterpart test on the
controller.

Everything else 9434 contributed to those files is kept: the thumbnail-gated preload (gating on
the full-resolution image held a stale latent preview on screen for the whole download), the
overlay clear on preload error as well as success, and its onInvocationComplete coverage.

CurrentImagePreview's wiring test is rewritten against the controller. Two of its four
assertions described the inlined implementation; one is now covered by a real unit test on the
controller (the marker is consumed on every rendered-item change even with no progress showing),
and the other becomes the same routing check CurrentVideoPreview already carries. The only test
dropped without a counterpart asserted that the decision function returned nothing but 'reveal'
or 'hide' -- a statement about an API that no longer exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merge resolution replaced invoke-ai#9434's inlined reveal with this branch's controller, and rewrote
the wiring test that went with it. That test carried the only assertion on either branch that the
component writes $isTemporarilyShowingSelectedImage -- it matched the literal hide path the
inlined version had -- and the rewrite dropped it.

Nothing else covers it. selectedItemReveal.test.ts substitutes its own setRevealed, so the
controller tests are structurally incapable of observing the atom, and CurrentVideoPreview's
assertions are all on the read side (withProgress, the metadata gate). Neither component is ever
mounted: this directory has no DOM test environment.

An adversarial review of the merge proved the gap by replacing setRevealed with a no-op in both
previews: all 26 assertions across the two wiring tests still passed, and so did the full suite,
with the reveal completely dead -- a mid-render gallery click doing nothing, which is the bug both
PRs exist to fix. Both tests now fail against that mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lstein lstein changed the title fix(ui): don't strand the viewer under the video progress overlay fix(ui): don't strand the viewer under the progress overlay, for images or videos Aug 20, 2026
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 20, 2026
…into the reveal machine

invoke-ai#9475 absorbed invoke-ai#9434, so this branch picks up invoke-ai#9434's thumbnail-gated preload, its error-path
overlay clear and its onInvocationComplete coverage, all of which merge cleanly onto the machine.

One conflict, in CurrentVideoPreview's wiring test: invoke-ai#9475 gained an assertion that the reveal is
actually connected to $isTemporarilyShowingSelectedImage, after an adversarial review showed the
whole suite stayed green with that wiring replaced by a no-op. Carried forward against this
branch's shape — the machine is built once in context.tsx for both previews, so the check moves
there and is made once rather than per component.

CurrentImagePreview's wiring test arrived from the merge still describing the controller this
branch replaced, and is rewritten against the machine: sync, the item-named readiness, attach,
and negative assertions that none of the three superseded implementations survive beside it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lstein

lstein commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated: this PR now contains #9434, which is closed in favour of it. The description is
rewritten to cover both — sections 5–7 are #9434's fixes, and there is a short account of how the
four conflicting files were resolved.

The short version: the reveal resolves to this branch's controller throughout. #9434 carried a
stateless decision function whose caller managed the previous-item ref, the auto-switch marker and
the timer by hand; the controller owns all of it and adds the resolve-window deferral and the
cleared-selection sentinel — which are the two inherited holes an earlier revision of this PR
listed as open, so they are fixed here rather than deferred. Everything else #9434 contributed
survives: the thumbnail-gated preload, the overlay clear on preload error, the duplicate-completion
dedupe.

An adversarial review of the resolution found one thing, and it was mine: the rewritten wiring test
dropped #9434's only assertion that the component writes $isTemporarilyShowingSelectedImage, and
nothing else covered it — the controller's tests substitute their own setRevealed, the video
tests only assert the read side, and neither component is ever mounted. Replacing that wiring with
a no-op in both previews left all 26 wiring assertions and the full suite green with the reveal
completely dead. Restored in f44c07a, with both tests now failing against that mutation.

One rule from #9434 deliberately did not survive, documented in the description: two changes of the
rendered item inside a single resolve window that end back where they started no longer reveal. Its
answer there was right for the wrong reason — the ref had advanced only because that version
overwrote it unconditionally, which is the bug that lost the first click in the much more common
single-change case.

#9520 is rebased on this and stays stacked; it is ~690 lines of restructuring on top.

@lstein
lstein requested a review from JPPhoto August 20, 2026 12:36

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge blockers:

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:92-94,567-587: retry timers survive socket/auth teardown; setEventListeners.tsx:77-82 and useSocketIO.ts:96-107 provide no disposal. A stale callback can fetch old output through the current store and mutate the new session's gallery/board caches. Effect: cross-session or foreign-content contamination. Likelihood: plausible during logout, account switch, or reconnect within 13 s. Recovery: reload or refetch caches. Test: fail the first DTO lookup, replace auth/socket, advance timers, assert no old fetch or dispatch.

  • invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts:151-155: first rendered item is always suppressed when lastRenderedItemNameRef is null. With progress visible, a normal first click selecting a video from an empty viewer therefore leaves the video behind the overlay. Effect: core video reveal path fails on first use. Likelihood: normal when generation is active before any item was rendered. Recovery: select another item or reopen the viewer. Test: mount with null prior render, select a video during progress, assert the temporary overlay is cleared.

Other findings/issues:

  • invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx:125-150,393-435: reveal timing starts from videoName, before loadeddata or canplay; preload="metadata" and near-zero seeking do not prove a decoded frame exists. Effect: slow videos can reveal as black/blank, then re-cover after the timer. Likelihood: plausible on slow storage/network or large videos. Recovery: wait for load, then reselect. Test: delay media readiness beyond the reveal timeout and assert the overlay remains cleared only after a frame is ready.

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:123-155,180-201: retry success unconditionally increments cached board totals, even if another refresh or delivery already inserted the output during the retry window. Name-list insertion dedupes, but board counts do not. Effect: stale or doubled board image counts until refetch. Likelihood: plausible during the 1-13 s retry window. Recovery: board refetch or reload. Test: prepopulate the output in board caches, run a failed lookup followed by retry success, assert totals remain unchanged.

Corner/impossible cases to note:

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:81-88,567-585: the 1000-entry LRU can evict a retry state before its timer fires; the callback then silently returns. Effect: bounded retry recovery is skipped and output remains absent. Likelihood: rare high-throughput/replay burst. Recovery: manual refetch or duplicate completion. Test: schedule a retry, process over 1000 distinct completions, advance the timer, assert the retry still runs.

Suggestions:

  • Instead of shared lastRenderedItemNameRef plus timing guards: use per-session idle, resolving(item_id), and live(item_id) states with an explicit selection-generation token. Add media-readiness state to distinguish initial render, user selection, auto-switch, rapid A/B/A, and decoded-frame availability.

  • Instead of leaving retry timers attached to an obsolete socket handler: return a disposer from setEventListeners, cancel timers during socket/auth teardown, and attach retries to an auth/session generation.

  • Instead of unconditional retry count updates: make board-total updates idempotent against current cached membership or perform one authoritative invalidation after delivery.

  • Instead of starting video reveal from videoName: gate reveal on loadeddata or canplay, with a bounded fallback for media that never becomes ready.

…t click

Two merge blockers and two findings from JPPhoto's fifth round.

Scheduled refetches survived socket and auth teardown. The timers close
over an event from the session that scheduled them but dispatch into
whatever store is current when they fire, so a logout, account switch or
reconnect inside the 13s window would fetch the old session's output and
insert it into the new session's gallery and board caches.
setEventListeners now returns a disposer, and useSocketIO calls it before
disconnecting.

The reveal suppressed the first item the viewer ever rendered, on the
grounds that the viewer opening onto an existing selection is not a
click. But a viewer sitting empty while a generation runs is a state the
user has been shown, and their first click there is a click like any
other — it was landing behind the overlay. An empty selection now records
the cleared-selection sentinel whether or not anything rendered before,
so that click reveals while the open-onto-a-selection render still does
not.

Also, since this is the third round it has come up: the video reveal no
longer starts its two seconds at mount. preload="metadata" and the
near-zero seek do not prove a frame exists, so the reveal could run out
over a black element and then re-cover it. The controller now holds the
claim until the item reports a decoded frame (onLoadedData), bounded by a
1s grace so media that never loads still makes the click land. Readiness
is reported as *which* item has painted rather than a boolean, because a
boolean would be reset from a different effect than the one that reads
it.

And retry success no longer bumps cached board totals: seconds after the
fact another refresh may already have inserted the output, and unlike the
name-list insert those increments do not dedupe. A retry now asks the
server for the affected boards instead.
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 21, 2026
…etry

Carried from invoke-ai#9475, where JPPhoto raised the first as a merge blocker;
this branch shares the completion handler.

- Scheduled refetches survived socket and auth teardown. They close over
  an event from the session that scheduled them but dispatch into
  whatever store is current when they fire, so a logout or account switch
  inside the retry window would insert the old session's output into the
  new session's caches. setEventListeners returns a disposer now, and
  useSocketIO calls it before disconnecting.

- Retry success no longer bumps cached board totals: by then another
  refresh may already have inserted the output, and unlike the name-list
  insert those increments do not dedupe. A retry asks the server for the
  affected boards instead.
@lstein

lstein commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

All four addressed at b874b90a4b.

Blocker — retry timers survive socket/auth teardown

Confirmed, and worse than "stale fetch": the timer closes over an event from the session that scheduled it, but dispatch resolves to whatever store is current when it fires. A logout or account switch inside the 13 s window would fetch the old session's output and insert it into the new session's gallery and board caches.

setEventListeners now returns a disposer, and useSocketIO calls it before socket.disconnect() — so the teardown that replaces the socket also drops anything that socket's handler scheduled. There is a behavioral test that a cancelled handler fires nothing afterwards, and a wiring test that the cleanup actually calls the disposer (deleting either fails).

Blocker — the first rendered item is always suppressed

Confirmed. The rule existed for the render that happens when the viewer opens onto an existing selection, which is not a click — but a viewer sitting empty while a generation runs is a state the user has already been shown, so their first click there is a click like any other, and it was landing behind the overlay.

An empty selection now records the cleared-selection sentinel whether or not anything rendered before it. That distinguishes the two: "nothing has ever rendered" still suppresses, "the viewer showed you nothing, and now you picked something" reveals. Both directions are tests.

Video reveal starting before a frame exists

You've raised this three times and I kept pointing at the follow-up branch; that was the wrong call, so it's fixed here. The controller now holds the claim instead of spending the two seconds: a reveal is owed when the selection lands and shown when the item reports a decoded frame (loadeddata), bounded by a 1 s grace so media that never becomes ready still makes the click land rather than swallowing it.

Readiness is reported as which item has painted, not a boolean — a boolean is reset from a different effect than the one that reads it, and a passive effect's setState does not reach the next effect's closure in the same commit, so a video→video click would have read the new name with the previous video's readiness.

Retry success double-counting board totals

Confirmed. Seconds after the fact another refresh or delivery may already have inserted the output; the name-list insert dedupes, the board totals are blind increments and do not. A retry now asks the server for the affected boards instead of bumping the cache — retries are rare enough that one authoritative refresh beats making every optimistic update idempotent.

Corner case: LRU eviction before the timer fires

Fixed too, since it was cheap: the missing names are captured in the timer's closure rather than read back from the LRU, so an eviction can no longer turn the refetch into a silent give-up. If the entry survives and says the work is done, the timer still stands down.

Suite is 1947 tests with tsc, eslint, prettier, knip and dpdm clean; every fix above mutation-checked rather than trusted green. #9434 carries the shared handler and socket-teardown changes as of e407aa6771.

@lstein
lstein requested a review from JPPhoto August 21, 2026 02:05

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge blockers:

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:563-581,660-665: teardown clears queued timers only; pending DTO deliveries continue and can schedule new retries after disposal. setEventListeners.tsx:1049-1051 has no closed/session guard. Effect: old-session DTOs can mutate the new user's gallery and board caches. Likelihood: plausible logout/account switch during a DTO request. Recovery: reload/refetch. Test: hold DTO lookup pending, call disposer, resolve failure, advance timers; current code performs a second lookup.

Other findings/issues:

  • invokeai/frontend/web/src/services/events/setEventListeners.test.ts:1-24: replaces the prior 644-line executable socket integration suite with two source-text checks. Workflow invalidation, queue cancellation, own/foreign routing, and node/progress isolation now have no runtime coverage. Effect: listener regressions can pass CI. Likelihood: every future edit to this shared listener, including this PR's lifecycle changes. Recovery: restore behavior-level socket tests. Test: retain a mock socket and trigger representative workflow, queue, foreign-event, and teardown paths.

Another corner/impossible case that can probably be left alone:

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:648-650: every retryable duplicate calls runTrackedDelivery(..., 0), resetting backoff. A persistent duplicate stream during DTO outage can start 1 s retry chains indefinitely. Effect: unbounded request/cache-invalidation load while output remains missing. Likelihood: rare replay/duplicate stream. Recovery: stops only when duplicates stop or page reloads. Test: repeatedly emit the same failed completion before each retry and assert globally bounded attempts.

Suggestions:

  • Instead of clearing only timer handles: mark the handler disposed with a session generation, check it before and after every await and before scheduleRetry, and abort in-flight RTKQ requests where possible.

  • Instead of source-string teardown checks: restore executable socket fixtures and test pending-lookup disposal, event routing, queue invalidation, and cross-user isolation.

  • Instead of resetting retry attempt to zero for every duplicate: preserve per-invocation attempt state while allowing one duplicate to supersede its queued timer.

… socket suite

Restores services/events/setEventListeners.test.ts, which I destroyed in
b874b90: I wrote the file without checking it existed, and its 644
lines of executable socket coverage — workflow invalidation, queue
cancellation, own/foreign routing, cross-user isolation — went with it,
replaced by two source-string checks. The suite is back, and the teardown
it needed is now an executable test in it rather than a grep: the mocked
handler carries a dispose(), and the disposer setEventListeners returns
is asserted to call it.

The blocker behind that test: disposal only cleared queued timers. A DTO
request already in flight came back afterwards, dispatched into whatever
store had replaced the old one, and scheduled fresh retries against it —
so a logout or account switch during a lookup could put one user's output
in the next user's gallery. The handler now knows it has been disposed
and checks after every await: nothing already fetched is dispatched,
nothing new is scheduled, and an event delivered after teardown does
nothing at all.

Also, a duplicate delivery restarted the backoff from one second, so a
stream of duplicates during an outage could keep starting fresh chains —
the bound existed but nothing was bounded by it. The attempt count lives
on the retry state now, and a duplicate resumes the chain where it had
got to.
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 21, 2026
… socket suite

Carried from invoke-ai#9475, where JPPhoto raised both; this branch shares the
completion handler and the socket listeners.

- setEventListeners.test.ts is restored: I destroyed its 644 lines of
  executable socket coverage by writing the file without checking it
  existed. The teardown assertion that replaced it is now an executable
  test inside the restored suite.
- Disposal only cleared queued timers, so a DTO request already in flight
  came back afterwards and dispatched into whatever store had replaced
  the old one. The handler now knows it is disposed and checks after
  every await.
- A duplicate delivery restarted the backoff from one second; the attempt
  count lives on the retry state now, so a duplicate resumes the chain.
@lstein

lstein commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

All three addressed at 042350c334.

The deleted socket suite — my mistake, and thank you for catching it

You're right, and it was worse than a trade-off: I wrote setEventListeners.test.ts with a redirect without checking whether the file existed, and 644 lines of executable socket coverage — workflow invalidation, queue cancellation, own/foreign routing, cross-user isolation — were destroyed by the write. That was careless, and the two source-string checks that replaced them were not an argument for doing it; I did not know I had done it.

The suite is restored intact, and the teardown coverage that motivated the file is now inside it as an executable test rather than a grep: the mocked completion handler carries a dispose(), and the disposer setEventListeners returns is asserted to call it. Deleting the return value fails that test.

Blocker — disposal only stopped queued timers

Confirmed, exactly as you described: the timer handles were cleared, but a DTO request already in flight came back afterwards, dispatched into whatever store had replaced the old one, and scheduled fresh retries against it.

The handler now knows it has been disposed and checks that after every await, not just before starting: an output fetched for the ended session is not dispatched, no retry is scheduled for it, and an event delivered after teardown does nothing at all. Your test recipe is one of the three new cases — hold the lookup pending, dispose, resolve the failure, advance: no second lookup, and nothing left armed. That last assertion is checked before advancing the clock, since a timer armed and then fired reads as "no timer" afterwards.

I did not go as far as aborting in-flight RTKQ requests. The guard makes their results inert, which covers the effect you named; cancelling the requests themselves would be a change to getImageDTOSafe's contract and I would rather do that deliberately than fold it in here.

Backoff reset by duplicates

Fixed rather than left alone — you were right that it makes the bound meaningless, and the fix is small. The attempt count lives on the retry state now, so a duplicate resumes the chain where it had got to instead of starting a fresh one at one second. A stream of duplicates through an outage gets one shared chain of three attempts, not three per duplicate.

Worth noting my first attempt at this did not work: I stored the attempt count from the pass doing the delivering, which for a duplicate is zero, so it overwrote the very thing it was meant to preserve. The mutation check caught it — the test passed against both the fix and its absence until I made the duplicates arrive after each retry fired, which is the shape you described.

Suite is 1972 tests with tsc, eslint, prettier, knip and dpdm clean. #9434 carries the same changes as of c5005b0e52.

@lstein
lstein requested a review from JPPhoto August 21, 2026 23:25
…xt round

A fresh adversarial pass over 042350c, done deliberately before the
reviewer's next round, converged on his known categories.

- An in-flight delivery disposed mid-fetch still ran $lastProgressEvent.set(null)
  after the gallery guards returned. That store is module-global across
  handler sessions, so a stale delivery resolving after a logout or
  account switch blanked the progress event the new session had put
  there. Guarded, and the disposal test now asserts the store is never
  touched.

- The DTO fetch loops kept issuing lookups after disposal — requests
  under the replacement session's credentials, cache writes into its
  store. Both loops now stop.

- Three load-bearing pieces had no test that failed without them, and
  test-insensitivity is where review rounds keep coming from:
  - the video path's post-fetch disposal guard (every disposal test used
    image events; its mutation survived the whole suite);
  - the reveal controller's resolve-window hold for an unpainted claim
    (deleting it silently re-created the swallowed-click failure);
  - the settle-listener registration in store.ts (every listener test
    builds its own store; deleting the registration failed nothing).
  Each now has a test pinned via mutation from a verified cwd — two of
  this session's mutation runs previously "passed" by running against a
  path that did not exist.
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 22, 2026
…9475

Carried across; this branch shares the completion handler.

- A delivery disposed mid-fetch still blanked the module-global
  $lastProgressEvent, wiping the next session's progress event.
- The DTO fetch loops kept issuing lookups after disposal.
- The video path's post-fetch disposal guard and the store.ts
  settle-listener registration had no test that failed without them;
  both are now pinned by mutation.

This branch keeps its pure reveal decision (the controller lives on
invoke-ai#9475); only the handler, its tests, and the store wiring test carry.

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix:

  • invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.test.ts:8-10 and invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.test.ts:6-12,15-86: tests inspect source text, not mounted components or real media/effect lifecycles. Effect: wiring/order regressions can leave the overlay or reveal broken while tests pass. Likelihood: normal future refactor. Recovery: manual browser repro. Test: add DOM/browser integration coverage for preload, media events, context swaps, and unmount.

Corner/impossible cases that can likely be ignored:

  • invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:103,664-698: after 1000 distinct completion keys, an older dedupe entry can be evicted; a delayed duplicate then repeats gallery work, board-count increments, and auto-switch. Effect: duplicate UI/state. Likelihood: rare long-lived session or replay burst. Recovery: refetch gallery and board data. Test: process 1001 unique completions, replay the first, assert no second delivery.

Suggestions:

  • Instead of source-string preview checks, use a mounted DOM/browser harness or a tested component adapter.

  • Instead of a fixed 1000-entry LRU as the sole duplicate guard, retain completion identity through the replay window or make gallery and board updates idempotent.

…or it

JPPhoto's round-7 finding, and a fair one: the preview components' tests
inspected source text, so a wiring or ordering regression could leave the
overlay or the reveal broken with every test green. The package had no
DOM test environment at all.

The wiring the two components shared — one controller per mount, run on
every input change with a cleanup that cancels only the timer, the flag
lowered on unmount — moves into useSelectedItemReveal, and the video's
painted-name readiness into usePaintedItemName beside it. Both components
become a hook call; the hook is mounted under happy-dom (new dev
dependency) with real effect lifecycles and mutation-verified coverage
for exactly the things source text cannot see:

- the image -> video component swap over the shared ref, including the
  outgoing component's timer being cancelled before it can cut the
  incoming reveal short;
- unmount lowering the flag with no timer left to re-raise it;
- StrictMode's double-invoked effects;
- readiness driven by a real <video> element's loadeddata event, and
  reset when the element is swapped for another video.

The remaining source-text assertions shrink to what they are good for:
pinning that the components actually call the tested hook, with the
right readiness expression on each path.
@github-actions github-actions Bot added the frontend-deps PRs that change frontend dependencies label Aug 22, 2026
@lstein

lstein commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed at f2853a635a (CI green).

Source-text tests → mounted DOM coverage

You're right, and this was the standing debt I'd been flagging each round: the package had no DOM test environment at all, so the component tests could only grep. Fixed via the adapter route you suggested:

  • The wiring both preview components share — one controller per mount, run on every input change with a cleanup that cancels only the timer, the flag lowered on unmount — is now useSelectedItemReveal, and the video's painted-name readiness is usePaintedItemName beside it. Each component becomes a hook call.
  • The hooks are mounted under happy-dom (new dev dependency) with real effect lifecycles. The suite covers exactly what source text cannot see: the image→video component swap over the shared ref — including the outgoing component's timer being cancelled before it can cut the incoming reveal short; unmount lowering the flag with no timer left to re-raise it; StrictMode's double-invoked effects; and readiness driven by a real <video> element's loadeddata event, reset when the element is swapped for another video.
  • Every one of those is mutation-verified: deleting the cleanup, the unmount lower, or the by-name readiness each fails a mounted test.

The remaining source-text assertions shrink to the one thing they're good for — pinning that the components actually call the tested hook, with the right readiness expression per path (preload-settled for images, loadeddata for videos).

What this does not cover, stated plainly: the full components still aren't mounted — their dnd/hotkeys/toast dependencies make that a project-infrastructure decision rather than a PR-sized one. The hook boundary means a component-level regression is now limited to "stopped calling the hook / fed it the wrong expression", which the residual text checks pin.

The LRU eviction corner

Agreed it can wait, and one note for the record: the retry path no longer depends on the entry surviving (the scheduled refetch captures its missing names at schedule time), and retry-path board totals are already server-authoritative rather than incremented. What remains exposed to a post-eviction replay is a first-delivery re-run — your "retain completion identity through the replay window" is the right shape for that, and I'd fold it into the same follow-up as making the optimistic inserts idempotent, rather than grow this PR further.

Suite is 1984 tests with tsc, eslint, prettier, knip and dpdm clean.

@lstein
lstein requested a review from JPPhoto August 22, 2026 15:29

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a very low probability issue of a delayed duplicate completion replaying after its 1000-entry dedupe record is evicted, causing duplicate gallery or board updates, but I'm approving this PR.

@lstein
lstein merged commit e431d24 into invoke-ai:main Aug 22, 2026
17 checks passed
@lstein
lstein deleted the fix/viewer-video-progress-overlay branch August 22, 2026 23:25
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 23, 2026
…nvoke-ai#9475

invoke-ai#9475 was squash-merged (e431d24), orphaning this stacked branch's
history, so this is a transplant rather than a rebase: one commit that
re-applies the branch's semantic delta on top of main's merged state —
including the parts of invoke-ai#9475 this branch predated (session disposal, the
restored socket suite, and the hook + happy-dom test architecture from
its final round).

What this branch changes, restated against main:

- Selections carry identity instead of being inferred by comparing names.
  $gallerySelection publishes {name, generation, isAutoSwitch} from one
  store listener; the auto-switch mark is set immediately before the
  handoff dispatches its selection and spent by that selection alone.
  The name-keyed marker module and its settle listener are deleted.

- The reveal is a state machine (idle | deferred | awaiting-media |
  revealing) owned by the viewer context — one instance for both preview
  components, so a click that switches media type is just another
  selection. The shared previous-item ref and its cleared-selection
  sentinel are gone. Re-picking the item already on screen now reveals:
  it changes no state and no rendered name, only the generation, which
  the old comparison could not see.

- The machine owns the revealed flag and its timers exclusively; the
  provider settles selections that land while neither preview is mounted
  and resets the machine on teardown.

- useSelectedItemReveal keeps its mounted happy-dom coverage, now
  asserting the machine wiring: one reveal running across a component
  swap on its original clock, attach making the provider's settle a
  no-op while mounted, the repeat-click reveal, media readiness from a
  real <video> loadeddata, and provider teardown leaving no timer.

Every wiring guarantee above is mutation-verified. The bounded-refetch
work this branch once carried merged with invoke-ai#9475 and is inherited, not
re-introduced.
lstein added a commit to lstein/InvokeAI that referenced this pull request Aug 23, 2026
…ial-failures-and-bounds

Import-block union in store.test.ts: invoke-ai#9475 added the auto-switch imports on
main while this branch added the workspace-purge test imports.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lstein added a commit that referenced this pull request Aug 24, 2026
… identity (#9520)

* refactor(ui): item-owned reveal machine, transplanted onto the merged #9475

#9475 was squash-merged (e431d24), orphaning this stacked branch's
history, so this is a transplant rather than a rebase: one commit that
re-applies the branch's semantic delta on top of main's merged state —
including the parts of #9475 this branch predated (session disposal, the
restored socket suite, and the hook + happy-dom test architecture from
its final round).

What this branch changes, restated against main:

- Selections carry identity instead of being inferred by comparing names.
  $gallerySelection publishes {name, generation, isAutoSwitch} from one
  store listener; the auto-switch mark is set immediately before the
  handoff dispatches its selection and spent by that selection alone.
  The name-keyed marker module and its settle listener are deleted.

- The reveal is a state machine (idle | deferred | awaiting-media |
  revealing) owned by the viewer context — one instance for both preview
  components, so a click that switches media type is just another
  selection. The shared previous-item ref and its cleared-selection
  sentinel are gone. Re-picking the item already on screen now reveals:
  it changes no state and no rendered name, only the generation, which
  the old comparison could not see.

- The machine owns the revealed flag and its timers exclusively; the
  provider settles selections that land while neither preview is mounted
  and resets the machine on teardown.

- useSelectedItemReveal keeps its mounted happy-dom coverage, now
  asserting the machine wiring: one reveal running across a component
  swap on its original clock, attach making the provider's settle a
  no-op while mounted, the repeat-click reveal, media readiness from a
  real <video> loadeddata, and provider teardown leaving no timer.

Every wiring guarantee above is mutation-verified. The bounded-refetch
work this branch once carried merged with #9475 and is inherited, not
re-introduced.

* fix(ui): anchor the media grace to the rendered item; stop counting selection mutations as picks

Two review findings from JPPhoto:

- The media grace deadline used to run from the moment the selection
  landed, so a slow DTO/render lifted the overlay onto whatever the
  component was still showing (the previous item, or nothing), and an
  item that took longer than the grace to arrive then stayed covered,
  its reveal already spent. The awaiting-media state now enters without
  a timer; the deadline is armed the first time the component actually
  renders the claimed item without a painted frame, so it bounds only
  the media wait it was designed for (failed loads, undecodable
  codecs). Until the item renders, the claim simply stays outstanding —
  cancelled as before by any newer selection or by the overlay going
  away.

- The gallery selection source counted every selectionChanged dispatch
  as the user picking something, so a multi-select mutation that leaves
  the active item in place — ctrl-clicking a non-active item off the
  selection — bumped the generation and flashed the overlay off for a
  gesture aimed at a different item. The two kinds of dispatch are
  indistinguishable at the listener (ctrl-deselecting `a` from [a, b]
  and plain-clicking `b` produce identical actions and transitions), so
  the separation happens where the gesture is known: plain clicks in
  the grid now dispatch imageSelected (reducer-equivalent for a single
  name), and selectionChanged is a pure mutation the listener no longer
  matches — its mutations count only when they move the active item.

Both behaviors are covered by new machine/listener tests, each verified
to fail against the code it fixes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ui): prune the post-delete selection instead of re-picking it

The delete flow's survivor branch — the displayed item outlived a deletion
that took other items out of the multi-selection — dispatched
`imageSelected(lastSelected)`. That leaves exactly the state it wants, but
`imageSelected` is the action that means "the user picked this", and the
viewer answers a pick by lifting a running generation's progress overlay off
the item for two seconds. Deleting some other item flashed a stale result
over a live render.

Dispatch `selectionChanged([lastSelected])` instead: same state, but the
mutation action, which the selection-source listener publishes only when it
moves the active item. The advance branch keeps `imageSelected` — there the
viewer genuinely moves to a different item, and revealing it is the point.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): let the board auto-select probe pick without reading as a user pick

Same class as the delete-flow pruning: the probe selects an item *for* the
user after a board or view change, but dispatched `imageSelected`, the action
that means "the user asked to see this". NoBoardBoard re-dispatches
boardIdSelected even when its board is already selected, so the probe can land
on the item already on screen — and as a pick that lifts a running
generation's progress overlay off it for two seconds.

`selectionChanged` leaves identical state and is published only when it moves
the active item, so a probe that genuinely changes what the viewer shows still
reveals; one that re-selects what is already displayed no longer does.

The test also covers the probe's success path, which had none.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): prune the live selection, not a stale snapshot of it

Adversarial review of the previous commit: switching the survivor branch to
`selectionChanged` is necessary but not sufficient. The branch collapsed the
selection onto `lastSelected`, snapshotted before the delete request. If the
user selects something else while the request is in flight, that collapse
discards their pick *and* moves the active item back — which the listener
publishes under its change-of-active-item clause, so the overlay flashes
anyway, for a gesture aimed at a third item.

Filter the deleted names out of the live selection instead, falling back to
the surviving displayed item when everything selected since has been deleted.
The ordinary (non-racing) case is unchanged.

Also strengthens both delete suites, which after the previous commit pinned
only the item left displayed and passed against a survivor branch that pruned
nothing at all, and narrows the doc comment that overstated the action choice
as sufficient on its own.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): stop no-op gallery navigation from discarding the selection

Adversarial review of the board-probe commit: guarding the probe's *action*
only helps when it happens to re-select the item already displayed. The probe
selects the board's first item unconditionally, so re-running it for a
navigation that changed nothing throws the user's selection away and moves the
viewer — which publishes as a change of active item and reveals over a running
generation anyway.

Guard the two callers that could re-dispatch for no change: NoBoardBoard
(GalleryBoard and VirtualBoardItem have always guarded this; it never did) and
the gallery's view tabs. NoBoardBoard's click actions move to their own module
so they can be unit-tested — the component pulls in the dnd stack, which a
plain unit test cannot import.

Also from the review, on the previous commit:
- both delete suites now assert *every* selection write, not just the first;
  a stray trailing dispatch — what the user would actually end up looking at —
  passed unnoticed, as did dropping the fallback from the video modal.
- adds the two paths neither suite covered: the video fallback, and a
  non-racing three-item multi-selection.
- the survivor branch's fallback cannot honour "don't move the active item"
  and can still name an item removed by an overlapping delete; both are
  pre-existing, and the comments now say so rather than claiming otherwise.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): decide "did anything actually change?" inside the auto-select probe

Replaces the click guards from the previous commit. Guarding each caller was
both too much and too little: too much, because it swallowed the click
outright, and clicking the board is exactly how a user recovers when the
selection is empty — after deleting the last item, after turning date boards
off (which resets the board and clears the selection without dispatching
boardIdSelected), or after the probe's own give-up. Too little, because the
guards read render-time snapshots, so a click racing a store update could be
dropped, and every future caller would have to remember the rule.

The probe itself knows the answer: if what the viewer is showing is still in
the list it just fetched, the board or view did not really change and there is
nothing to fix, so it writes nothing. If the selection is empty or gone from
the list, it selects as before. One place, live state, all callers.

Also fixes the give-up path this exposed: `condition` only re-evaluates its
predicate when an action is dispatched, so an already-fulfilled list with a
quiet store gave no wake-up at all — the probe sat through its 5 s deadline
and cleared a selection it should have kept.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* test(ui): pin the probe's mutation-action choice where it is observable

With the probe now skipping writes that change nothing, every write it does
make moves the displayed item and would publish under either action — so the
suite passed with the probe dispatching `imageSelected` again. An empty board
with nothing selected is the one write that changes nothing and would still
publish as a pick; that case is now covered.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): key the probe's skip on whether the navigation changed anything

Adversarial review of the previous two commits found the "is the displayed
item in the new list?" test wrong in two ways.

It skips genuine board switches. A virtual date board's query args drop
`board_id` and filter on `created_date` alone, so its list is a superset of
every board's items for that day: switching from a real board to a date board
left the viewer on the old board's item, with the grid scrolled to it instead
of to the newest — and a cross-board multi-selection survived the switch, so
bulk actions then operated on items not visible in the grid.

And the short-circuit that skipped the query wait for an already-fulfilled
list made the effect run to completion synchronously inside the dispatch that
started it. That let the probe's write land between
`markNextSelectionAutoSwitched()` and the auto-switch's own `imageSelected`,
consuming the marker so the auto-switch registered as a user pick — the exact
flash the reveal machine exists to prevent. Only an incidental cache
invalidation kept it from firing today. Reverted; the pre-existing quiet-store
give-up it was fixing is reported to the reviewer instead.

The question the probe actually wants is whether the board or view changed at
all, which `getOriginalState()` answers exactly, before any await — so a click
that should do nothing cannot reach the give-up branch either. An empty
selection still probes, because that is the case where the user needs the
probe to pick for them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): require the displayed item to be in the list before skipping a probe

Adversarial review of the previous commit: "the selection is non-empty" is not
"the user has something valid on screen". A search term — or starredFirst,
orderDir, the archived-boards toggle — narrows the list without starting a
probe, so the selection can be absent from what the grid shows. Clicking the
board, which is how the user gets out of that, was being swallowed. The same
gap stranded the viewer on an item from a board that had just been deleted:
the app dispatches `boardIdSelected` then `galleryViewChanged` back to back
there (and again when an upload targets another board), and the second of each
pair is a no-op, so it skipped while the first board change was still in
flight.

Ask the fuller question instead: did this navigation change the board or view,
and is the displayed item in the list that board is already showing? An
uncached list falls through and probes as before. Read synchronously off the
cache, so a click that should do nothing never enters the query wait, where a
quiet store can expire the 5s deadline and clear the selection.

The check also moves above `cancelActiveListeners()`. That is defensive rather
than load-bearing — with membership required, a skip can only happen when the
displayed item is valid for the list now on screen, so cancelling first would
not actually strand anything, and no test can tell the two orders apart — but
a navigation that changes nothing has no business cancelling the probe of one
that did.

Adds the coverage the review found missing: the cancel-then-return pair, the
view-tab half of the check (a mutation neutering it passed the whole suite),
the filtered-out-selection recovery, and a switch whose destination list is
cached *before* the click — the case that distinguishes comparing against the
previous state from a bare membership test. The empty-selection test no longer
leans on a synthetic tick to wake the probe.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* revert(ui): drop the probe's no-op-navigation skip from this PR

Five attempts, four adversarial reviews, a new regression each time — the
auto-select probe's interaction with RTK Query's cache, `condition`'s
wake-on-dispatch semantics, and the action pairs the app dispatches around it
is subtler than it looks from the call site.

The last version could still be provoked into the opposite failure: a click
that changed nothing, on a selection the current list does not contain, fell
through to a query wait that nothing would wake, and the 5 s give-up cleared
the selection outright. A cold cache entry — which toggling starred-first or
order direction produces instantly — let the same click take a good selection
away and lift the progress overlay, the very flash this was meant to stop.

None of this is what the review that started this round was about, and all of
it predates the PR. Keeping only the part that does belong: the probe writes
with the mutation action, so a re-run that lands back on the item already
displayed no longer announces itself as a user pick. Filed the rest.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

* fix(ui): type the probe test's query-args call against RootState

CI's `tsc --noEmit` covers the whole project; vitest's typecheck pass only
looks at `*.test-d.ts`, so this never showed up locally. The test store holds
just the two slices the listener needs, and the selector is typed against the
full RootState.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KRgbe8C7ZfunjxRuFZxhPw

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.0 frontend PRs that change frontend files frontend-deps PRs that change frontend dependencies

Projects

Status: 6.14.x Theme: USER EXPERIENCE

Development

Successfully merging this pull request may close these issues.

2 participants