diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts deleted file mode 100644 index 54c1ff2bde2..00000000000 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.test.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'; -import type { AppStartListening } from 'app/store/store'; -import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; -import { - boardIdSelected, - gallerySliceConfig, - imageSelected, - selectionChanged, -} from 'features/gallery/store/gallerySlice'; -import { beforeEach, describe, expect, it } from 'vitest'; - -import { addAutoSwitchedSelectionListener } from './autoSwitchedSelection'; - -// A store with the real gallery reducer and the real listener, so the predicate is exercised -// against actual selection-writing actions rather than a hand-built state pair. -const buildStore = () => { - const listenerMiddleware = createListenerMiddleware(); - addAutoSwitchedSelectionListener(listenerMiddleware.startListening as unknown as AppStartListening); - return configureStore({ - reducer: { gallery: gallerySliceConfig.slice.reducer }, - middleware: (getDefaultMiddleware) => getDefaultMiddleware().prepend(listenerMiddleware.middleware), - }); -}; - -describe('addAutoSwitchedSelectionListener', () => { - beforeEach(() => { - // The marker is a module singleton; drop anything a previous test left on it. - autoSwitchedImages.settle(null); - }); - - it('keeps the marker when the auto-switch selection lands', () => { - const store = buildStore(); - autoSwitchedImages.record('a.png'); - store.dispatch(imageSelected('a.png')); - expect(autoSwitchedImages.consume('a.png')).toBe(true); - }); - - it('drops the marker once the user selects something else', () => { - // The dead click this exists to prevent: the auto-switch to A never rendered because the user - // clicked B first, so their later click on A must still get its reveal. - const store = buildStore(); - autoSwitchedImages.record('a.png'); - store.dispatch(imageSelected('a.png')); - store.dispatch(imageSelected('b.png')); - store.dispatch(imageSelected('a.png')); - expect(autoSwitchedImages.consume('a.png')).toBe(false); - }); - - it('settles on every action that writes the selection, not just imageSelected', () => { - const store = buildStore(); - - autoSwitchedImages.record('a.png'); - store.dispatch(imageSelected('a.png')); - store.dispatch(selectionChanged(['b.png'])); - expect(autoSwitchedImages.consume('a.png')).toBe(false); - - autoSwitchedImages.record('c.png'); - store.dispatch(imageSelected('c.png')); - store.dispatch(boardIdSelected({ boardId: 'other', select: { selection: ['d.png'], galleryView: 'images' } })); - expect(autoSwitchedImages.consume('c.png')).toBe(false); - }); - - it('leaves the marker alone when an action does not move the selection', () => { - const store = buildStore(); - autoSwitchedImages.record('a.png'); - store.dispatch(imageSelected('a.png')); - // Selecting the same item again, and a board switch that carries no selection, must not - // discard a marker whose image has not rendered yet. - store.dispatch(imageSelected('a.png')); - store.dispatch(boardIdSelected({ boardId: 'other' })); - expect(autoSwitchedImages.consume('a.png')).toBe(true); - }); -}); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts deleted file mode 100644 index 0b14616f0b3..00000000000 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection.ts +++ /dev/null @@ -1,27 +0,0 @@ -import type { AppStartListening } from 'app/store/store'; -import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; -import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; - -/** - * Keeps the auto-switch marker scoped to the selection it was recorded for. - * - * onInvocationComplete records the item it is about to auto-switch to, so the viewer's reveal - * effect can tell that handoff apart from a user's gallery click. The marker is only meaningful - * while that selection stands: once the selection moves on, the recorded auto-switch will never - * render, and leaving the marker behind would make the user's next click on that item read as an - * auto-switch and get no reveal. - * - * Matched by state rather than by action type on purpose — the selection is written by several - * reducers (imageSelected, selectionChanged, boardIdSelected, comparedImagesSwapped, - * showVirtualBoardsChanged, logout), and a new one added later would silently escape an - * action-type list, leaving exactly the stale marker this exists to prevent. - */ -export const addAutoSwitchedSelectionListener = (startAppListening: AppStartListening) => { - startAppListening({ - predicate: (_action, currentState, previousState) => - selectLastSelectedItem(currentState) !== selectLastSelectedItem(previousState), - effect: (_action, { getState }) => { - autoSwitchedImages.settle(selectLastSelectedItem(getState()) ?? null); - }, - }); -}; diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts index 32d92de52d9..1ffade0f411 100644 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.test.ts @@ -1,28 +1,38 @@ import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'; -import type { AppStartListening } from 'app/store/store'; +import type { AppStartListening, RootState } from 'app/store/store'; +import { $gallerySelection, resetGallerySelectionSource } from 'features/gallery/store/gallerySelectionSource'; +import { selectGalleryItemNamesQueryArgs } from 'features/gallery/store/gallerySelectors'; import { + boardIdSelected, gallerySliceConfig, galleryViewChanged, imageSelected, selectionChanged, } from 'features/gallery/store/gallerySlice'; import { api } from 'services/api'; +import { galleryApi } from 'services/api/endpoints/gallery'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { addBoardIdSelectedListener } from './boardIdSelected'; +import { addGallerySelectionSourceListener } from './gallerySelectionSource'; // The listener waits for the board's item list before auto-selecting, so the store needs the API -// slice present (the query is never fulfilled here — the point is what happens meanwhile). -const buildStore = () => { +// slice present (in most tests here the query is never fulfilled — the point is what happens +// meanwhile). `withSelectionSource` also registers the listener that publishes selections to the +// viewer, for the tests that care whether the probe's own write reads as a user pick. +const buildStore = ({ withSelectionSource = false }: { withSelectionSource?: boolean } = {}) => { const listenerMiddleware = createListenerMiddleware(); addBoardIdSelectedListener(listenerMiddleware.startListening as unknown as AppStartListening); + if (withSelectionSource) { + addGallerySelectionSourceListener(listenerMiddleware.startListening as unknown as AppStartListening); + } return configureStore({ reducer: { gallery: gallerySliceConfig.slice.reducer, [api.reducerPath]: api.reducer, }, middleware: (getDefaultMiddleware) => - getDefaultMiddleware({ serializableCheck: false }).prepend(listenerMiddleware.middleware), + getDefaultMiddleware({ serializableCheck: false }).prepend(listenerMiddleware.middleware).concat(api.middleware), }); }; @@ -64,8 +74,8 @@ describe('addBoardIdSelectedListener', () => { }); it('does not overwrite a selection made through the gallery grid either', () => { - // Thumbnail clicks and keyboard navigation dispatch selectionChanged, not imageSelected, so - // matching on the action type alone leaves the ordinary path exposed. + // Ctrl/shift-clicks and the delete flow's selection pruning dispatch selectionChanged, not + // imageSelected, so matching on the action type alone leaves those paths exposed. const store = buildStore(); store.dispatch(galleryViewChanged('images')); @@ -114,4 +124,49 @@ describe('addBoardIdSelectedListener', () => { expect(store.getState().gallery.selection).toEqual(['a.png']); }); }); + + it("auto-selects the board's first item without that reading as a user pick", async () => { + // The probe picks *for* the user, so its write must not publish as a pick when it lands on the + // item already displayed: NoBoardBoard re-dispatches boardIdSelected even when its board is + // already selected, and the viewer answers a pick by lifting a running generation's progress + // overlay off the item for two seconds — a stale flash for a click on the current board. + resetGallerySelectionSource(); + const store = buildStore({ withSelectionSource: true }); + + store.dispatch(boardIdSelected({ boardId: 'none' })); + // Fulfil the item-name query the probe is waiting on, under the same cache key it computes. + // The store here carries only the two slices this listener needs, so the selector — typed + // against the whole RootState — has to be told that is enough. + const queryArgs = selectGalleryItemNamesQueryArgs(store.getState() as unknown as RootState); + // The upsert has to be flushed through the fake timers before it is awaited, or its fulfilled + // action lands after the probe's 5s give-up. + const upsert = store.dispatch( + galleryApi.util.upsertQueryData('listGalleryItemNames', queryArgs, { + item_names: ['already-showing.png'], + starred_count: 0, + total_count: 1, + }) + ); + await vi.advanceTimersByTimeAsync(0); + await upsert; + await vi.advanceTimersByTimeAsync(6000); + + // The probe really does select for the user — this is also the only coverage of that path. + expect(store.getState().gallery.selection).toEqual(['already-showing.png']); + const generationAfterFirstProbe = $gallerySelection.get().generation; + expect(generationAfterFirstProbe, 'moving the viewer to a new item is worth publishing').toBeGreaterThan(0); + + // Re-select the same board. The probe runs again and lands on the item already displayed. + store.dispatch(boardIdSelected({ boardId: 'none' })); + // The listener's `condition` only re-evaluates its predicate when an action is dispatched, so a + // test that merely advances time would watch this second probe time out and clear the selection + // — the give-up path, not the path under test. Any action wakes it; this one touches nothing. + store.dispatch({ type: 'test/tick' }); + await vi.advanceTimersByTimeAsync(6000); + + expect(store.getState().gallery.selection).toEqual(['already-showing.png']); + expect($gallerySelection.get().generation, 'nothing moved, so there is nothing to reveal').toBe( + generationAfterFirstProbe + ); + }); }); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts index 01072a81162..10a66b17720 100644 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/boardIdSelected.ts @@ -1,7 +1,7 @@ import { isAnyOf } from '@reduxjs/toolkit'; import type { AppStartListening } from 'app/store/store'; import { selectGalleryItemNamesQueryArgs, selectSelection } from 'features/gallery/store/gallerySelectors'; -import { boardIdSelected, galleryViewChanged, imageSelected } from 'features/gallery/store/gallerySlice'; +import { boardIdSelected, galleryViewChanged, selectionChanged } from 'features/gallery/store/gallerySlice'; import { galleryApi } from 'services/api/endpoints/gallery'; /** The actions that ask this listener to pick an item for the user. */ @@ -11,10 +11,11 @@ export const addBoardIdSelectedListener = (startAppListening: AppStartListening) startAppListening({ // Two jobs, so this cannot be a plain action matcher. The probe below is started by a board or // view change — but it must also be *cancelled* by any selection that lands while it waits, - // and a selection arrives through several actions: imageSelected from the gallery's auto-switch - // and keyboard navigation, selectionChanged from thumbnail clicks, boardIdSelected carrying a - // selection. Matching the resulting change of the selection covers all of them, including any - // writer added later — an action list would silently miss it. + // and a selection arrives through several actions: imageSelected from the gallery's auto-switch, + // plain thumbnail clicks and keyboard navigation, selectionChanged from ctrl/shift-clicks, the + // delete flow's pruning and this listener's own probe, boardIdSelected carrying a selection. + // Matching the resulting change of the selection covers all of them, including any writer added + // later — an action list would silently miss it. // // The whole selection, not just its active item: removing one of several selected thumbnails, // or re-picking the one already active, leaves the last item unchanged while still being the @@ -53,15 +54,30 @@ export const addBoardIdSelectedListener = (startAppListening: AppStartListening) // must use getState() to ensure we do not have stale state const isSuccess = await condition(() => selectQuery(getState()).isSuccess, 5000); + // The probe picks an item *for* the user, so it writes the selection with the mutation + // action rather than `imageSelected`. The state is identical either way, but `imageSelected` + // means "the user asked to see this", and while a generation is running the viewer answers + // that by lifting the progress overlay off the item for a couple of seconds — so a write + // that changes nothing must not announce itself as a pick. NoBoardBoard and the view tabs + // dispatch even when nothing changed (unlike GalleryBoard and VirtualBoardItem), which + // re-runs this probe; when it lands back on the item already displayed, the mutation action + // is what keeps it silent. A write that genuinely moves the displayed item still reveals, + // through the change-of-active-item clause. See gallerySelectionSource. + // + // This does NOT stop that re-run from *replacing* a selection further down the list with + // `item_names[0]` — a real bug, but an older and wider one than this file, tracked in its own + // issue along with the give-up branch below clearing a good selection whenever `condition` + // gets no wake-up within 5s. if (!isSuccess) { - dispatch(imageSelected(null)); + dispatch(selectionChanged([])); return; } // the board was just changed - we can select the first gallery item (image or video) const itemNames = selectQuery(getState()).data?.item_names; + const firstItemName = itemNames?.[0]; - dispatch(imageSelected(itemNames?.[0] ?? null)); + dispatch(selectionChanged(firstItemName ? [firstItemName] : [])); }, }); }; diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.test.ts new file mode 100644 index 00000000000..96b57f8a806 --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.test.ts @@ -0,0 +1,97 @@ +import { configureStore, createListenerMiddleware } from '@reduxjs/toolkit'; +import type { AppStartListening } from 'app/store/store'; +import { + $gallerySelection, + markNextSelectionAutoSwitched, + resetGallerySelectionSource, +} from 'features/gallery/store/gallerySelectionSource'; +import { + boardIdSelected, + gallerySliceConfig, + imageSelected, + selectionChanged, +} from 'features/gallery/store/gallerySlice'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { addGallerySelectionSourceListener } from './gallerySelectionSource'; + +const buildStore = () => { + const listenerMiddleware = createListenerMiddleware(); + addGallerySelectionSourceListener(listenerMiddleware.startListening as unknown as AppStartListening); + return configureStore({ + reducer: { gallery: gallerySliceConfig.slice.reducer }, + middleware: (getDefaultMiddleware) => getDefaultMiddleware().prepend(listenerMiddleware.middleware), + }); +}; + +describe('addGallerySelectionSourceListener', () => { + beforeEach(() => { + resetGallerySelectionSource(); + }); + + it('publishes a click made through the gallery grid', () => { + const store = buildStore(); + store.dispatch(imageSelected('clicked.png')); + expect($gallerySelection.get()).toMatchObject({ name: 'clicked.png', isAutoSwitch: false }); + }); + + it('does not publish a multi-select mutation that leaves the active item in place', () => { + // Active item b, selection [a, b]: ctrl-clicking `a` off the selection dispatches + // selectionChanged([b]). Nothing the viewer shows changes — publishing it would flash the + // progress overlay off for bookkeeping aimed at a different item. + const store = buildStore(); + store.dispatch(selectionChanged(['a.png', 'b.png'])); + const beforeDeselect = $gallerySelection.get().generation; + + store.dispatch(selectionChanged(['b.png'])); + + expect($gallerySelection.get().generation).toBe(beforeDeselect); + }); + + it('publishes a multi-select mutation that moves the active item', () => { + // Ctrl-clicking an unselected item appends it and makes it active. selectionChanged is not in + // the pick list, so this relies on the change-of-active-item clause. + const store = buildStore(); + store.dispatch(imageSelected('a.png')); + store.dispatch(selectionChanged(['a.png', 'b.png'])); + expect($gallerySelection.get()).toMatchObject({ name: 'b.png', isAutoSwitch: false }); + }); + + it('publishes a re-selection of the item already active as a new selection', () => { + // Nothing in the state changes, so a state-transition-only predicate would miss it — and the + // viewer would have no way to make a repeat click on the displayed item visible. + const store = buildStore(); + store.dispatch(imageSelected('a.png')); + const first = $gallerySelection.get().generation; + store.dispatch(imageSelected('a.png')); + expect($gallerySelection.get().generation).toBeGreaterThan(first); + }); + + it('attributes an auto-switch that carries its own board change', () => { + const store = buildStore(); + markNextSelectionAutoSwitched(); + store.dispatch(boardIdSelected({ boardId: 'other', select: { selection: ['auto.png'], galleryView: 'images' } })); + expect($gallerySelection.get()).toMatchObject({ name: 'auto.png', isAutoSwitch: true }); + }); + + it('publishes a selection cleared by an action it does not name', () => { + // logout is not in the action list; the active-item clause is what covers it. + const store = buildStore(); + store.dispatch(imageSelected('a.png')); + store.dispatch({ type: 'auth/logout' }); + expect($gallerySelection.get().name).toBeNull(); + }); + + it('does not treat a bare board click as the user picking the item that stays selected', () => { + // Clicking a board in the boards list dispatches boardIdSelected with no selection payload and + // leaves the selection alone. Counting it would make the viewer reveal an item the user never + // clicked, over the live progress preview. + const store = buildStore(); + store.dispatch(imageSelected('a.png')); + const beforeBoardClick = $gallerySelection.get().generation; + + store.dispatch(boardIdSelected({ boardId: 'some-other-board' })); + + expect($gallerySelection.get().generation).toBe(beforeBoardClick); + }); +}); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.ts new file mode 100644 index 00000000000..1203181bd6b --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource.ts @@ -0,0 +1,59 @@ +import type { UnknownAction } from '@reduxjs/toolkit'; +import { isAnyOf } from '@reduxjs/toolkit'; +import type { AppStartListening } from 'app/store/store'; +import { recordGallerySelection } from 'features/gallery/store/gallerySelectionSource'; +import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; +import { boardIdSelected, comparedImagesSwapped, imageSelected } from 'features/gallery/store/gallerySlice'; + +/** + * The actions through which a user (or the auto-switch) picks something. + * + * `boardIdSelected` only counts when it carries a selection: clicking a board in the boards list + * dispatches it bare, leaves the selection alone, and must not read as the user picking the item + * that happens to still be selected — the viewer would reveal an item they never clicked. + * + * `selectionChanged` is deliberately absent: it is the multi-selection *mutation* action + * (ctrl/shift-clicks, bulk operations, the delete flow pruning deleted names out of the + * selection), and a mutation that leaves the active item in place — ctrl-clicking a non-active + * item off the selection, say — is bookkeeping, not the user asking to see the item that stays + * active; counting it would flash the progress overlay off for a gesture aimed at a different + * item. A mutation that *moves* the active item is caught by the change-of-active-item clause + * below, and a plain click dispatches `imageSelected`, so the deliberate re-pick of the + * already-active item still lands here. + * + * The corollary binds the writers, not just this file: code that rewrites the selection without + * the user having asked for anything must use `selectionChanged`, even where `imageSelected` + * would leave identical state — when the write leaves the active item where it is, the action is + * the only thing left to distinguish "the user picked this" from "this happens to still be + * selected". Choosing the action is necessary but not sufficient, though: the clause below still + * publishes if the write *moves* the active item, so such a writer must also leave the active item + * alone rather than collapsing the selection onto a stale snapshot of it. See the delete modals' + * survivor branch, which does both, and its fallback, which fires only when everything selected has + * been deleted and cannot honour the second half — so it does reveal. The board auto-select probe + * in listeners/boardIdSelected.ts honours the first half only: it is silent when it lands back on + * the item already displayed, but a re-run for a navigation that changed nothing still replaces a + * selection further down the list, which is tracked separately. + */ +const isSelectionDispatch = (action: UnknownAction): boolean => + isAnyOf(imageSelected, comparedImagesSwapped)(action) || + (boardIdSelected.match(action) && action.payload.select !== undefined); + +/** + * Publishes every gallery selection to $gallerySelection, so the viewer can tell a user's click + * from the gallery auto-switching to a finished item, and one selection from the next. + * + * The predicate has two clauses because neither alone is sufficient. Matching the selection + * *actions* catches re-selecting the item already active — a real event that changes no state, and + * the one the reveal needs in order to make a repeat click visible. Matching a *change of active + * item* catches every other writer, including ones added later that an action list would miss + * (`showVirtualBoardsChanged` and `logout` both clear the selection today). + */ +export const addGallerySelectionSourceListener = (startAppListening: AppStartListening) => { + startAppListening({ + predicate: (action, currentState, previousState) => + isSelectionDispatch(action) || selectLastSelectedItem(currentState) !== selectLastSelectedItem(previousState), + effect: (_action, { getState }) => { + recordGallerySelection(selectLastSelectedItem(getState()) ?? null); + }, + }); +}; diff --git a/invokeai/frontend/web/src/app/store/store.test.ts b/invokeai/frontend/web/src/app/store/store.test.ts index 056bc639b48..e98155b8aee 100644 --- a/invokeai/frontend/web/src/app/store/store.test.ts +++ b/invokeai/frontend/web/src/app/store/store.test.ts @@ -4,7 +4,11 @@ import { externalTokenAdopted, logout, sessionExpiredLogout, setCredentials } fr import { isModalOpenChanged, videosToChangeSelected } from 'features/changeBoardModal/store/slice'; import { positivePromptChanged } from 'features/controlLayers/store/paramsSlice'; import { deleteVideosWithDialog } from 'features/deleteVideoModal/store/state'; -import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; +import { + $gallerySelection, + markNextSelectionAutoSwitched, + resetGallerySelectionSource, +} from 'features/gallery/store/gallerySelectionSource'; import { autoAddBoardIdChanged, boardIdSelected, @@ -124,20 +128,19 @@ describe('auth cache isolation', () => { }); describe('gallery listener registration', () => { - it('settles the auto-switch marker through the real store wiring', () => { + it('publishes selections through the real store wiring', () => { // The per-listener tests build their own store, so nothing else fails if the registration in - // store.ts is deleted — and without it the marker never settles, every stale marker suppresses - // the user's next click on that item, and the exact dead click the marker exists to prevent - // comes back. This is the one test that dispatches through createStore()'s own listeners. + // store.ts is deleted — and without it no selection is ever published: the auto-switch mark is + // never spent, and the viewer's reveal machine never hears about any selection at all. This is + // the one test that dispatches through createStore()'s own listeners. const store = createStore(); - autoSwitchedImages.settle(null); // module singleton; start from empty + resetGallerySelectionSource(); // module singleton; start from empty - autoSwitchedImages.record('auto-switched.png'); + markNextSelectionAutoSwitched(); store.dispatch(imageSelected('auto-switched.png')); - store.dispatch(imageSelected('user-clicked-elsewhere.png')); + expect($gallerySelection.get()).toMatchObject({ name: 'auto-switched.png', isAutoSwitch: true }); - expect(autoSwitchedImages.consume('auto-switched.png'), 'the marker must not survive the selection moving on').toBe( - false - ); + store.dispatch(imageSelected('user-clicked.png')); + expect($gallerySelection.get()).toMatchObject({ name: 'user-clicked.png', isAutoSwitch: false }); }); }); diff --git a/invokeai/frontend/web/src/app/store/store.ts b/invokeai/frontend/web/src/app/store/store.ts index 7ea4ffbf45c..ea57993bf4c 100644 --- a/invokeai/frontend/web/src/app/store/store.ts +++ b/invokeai/frontend/web/src/app/store/store.ts @@ -12,11 +12,11 @@ import { errorHandler } from 'app/store/enhancers/reduxRemember/errors'; import { addAdHocPostProcessingRequestedListener } from 'app/store/middleware/listenerMiddleware/listeners/addAdHocPostProcessingRequestedListener'; import { addAnyEnqueuedListener } from 'app/store/middleware/listenerMiddleware/listeners/anyEnqueued'; import { addAppStartedListener } from 'app/store/middleware/listenerMiddleware/listeners/appStarted'; -import { addAutoSwitchedSelectionListener } from 'app/store/middleware/listenerMiddleware/listeners/autoSwitchedSelection'; import { addBatchEnqueuedListener } from 'app/store/middleware/listenerMiddleware/listeners/batchEnqueued'; import { addDeleteBoardAndImagesFulfilledListener } from 'app/store/middleware/listenerMiddleware/listeners/boardAndImagesDeleted'; import { addBoardIdSelectedListener } from 'app/store/middleware/listenerMiddleware/listeners/boardIdSelected'; import { addBulkDownloadListeners } from 'app/store/middleware/listenerMiddleware/listeners/bulkDownload'; +import { addGallerySelectionSourceListener } from 'app/store/middleware/listenerMiddleware/listeners/gallerySelectionSource'; import { addGetOpenAPISchemaListener } from 'app/store/middleware/listenerMiddleware/listeners/getOpenAPISchema'; import { addImageAddedToBoardFulfilledListener } from 'app/store/middleware/listenerMiddleware/listeners/imageAddedToBoard'; import { addImageRemovedFromBoardFulfilledListener } from 'app/store/middleware/listenerMiddleware/listeners/imageRemovedFromBoard'; @@ -327,7 +327,7 @@ addImageAddedToBoardFulfilledListener(startAppListening); addImageRemovedFromBoardFulfilledListener(startAppListening); addBoardIdSelectedListener(startAppListening); addArchivedOrDeletedBoardListener(startAppListening); -addAutoSwitchedSelectionListener(startAppListening); +addGallerySelectionSourceListener(startAppListening); // Node schemas addGetOpenAPISchemaListener(startAppListening); diff --git a/invokeai/frontend/web/src/features/deleteImageModal/store/state.test.ts b/invokeai/frontend/web/src/features/deleteImageModal/store/state.test.ts index 385482447cc..298ef013a56 100644 --- a/invokeai/frontend/web/src/features/deleteImageModal/store/state.test.ts +++ b/invokeai/frontend/web/src/features/deleteImageModal/store/state.test.ts @@ -32,6 +32,7 @@ vi.mock('features/gallery/store/gallerySelectors', () => ({ vi.mock('features/gallery/store/gallerySlice', () => ({ imageSelected: vi.fn((payload: string | null) => ({ type: 'gallery/imageSelected', payload })), + selectionChanged: vi.fn((payload: string[]) => ({ type: 'gallery/selectionChanged', payload })), })); vi.mock('features/system/store/systemSlice', () => ({ @@ -58,34 +59,63 @@ vi.mock('features/gallery/store/selectCachedGalleryItemNames', async (importOrig import type { AppStore } from 'app/store/store'; import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; +import { imageSelected } from 'features/gallery/store/gallerySlice'; import { selectCachedGalleryItemNames } from 'features/gallery/store/selectCachedGalleryItemNames'; import { handleDeletions } from './state'; -const buildStore = (selection: string[], failingNames: Set) => { +/** + * `selectionDuringDelete` re-points the store's selection when the delete request is issued, + * standing in for the user selecting something else while it is in flight. The rest of + * handleDeletions then runs against that newer selection, as it does in the app. + * + * It writes `currentSelection` directly rather than dispatching, so the simulated gesture does not + * land in `dispatched` and cannot be mistaken for the production write under test. The seam is + * only equivalent to a real mid-flight click because neither modal reads state between issuing the + * request and the post-await block; anything added in between would need a real dispatch here. + */ +const buildStore = (selection: string[], failingNames: Set, selectionDuringDelete?: string[]) => { const dispatched: unknown[] = []; + let currentSelection = selection; const dispatch = vi.fn((action: unknown) => { dispatched.push(action); const typed = action as { type?: string; image_names?: string[] }; if (typed?.type === 'imagesApi/deleteImages') { return { - unwrap: () => - Promise.resolve({ + unwrap: () => { + if (selectionDuringDelete) { + currentSelection = selectionDuringDelete; + } + return Promise.resolve({ deleted_images: (typed.image_names ?? []).filter((name) => !failingNames.has(name)), affected_boards: [], - }), + }); + }, }; } return action; }); - const getState = vi.fn(() => ({ gallery: { selection }, nodes: { present: { nodes: [] } } })); + const getState = vi.fn(() => ({ gallery: { selection: currentSelection }, nodes: { present: { nodes: [] } } })); return { store: { dispatch, getState } as unknown as AppStore, dispatched }; }; -const getSelectionChange = (dispatched: unknown[]) => - dispatched.find( - (action): action is { type: string; payload: string | null } => - !!action && typeof action === 'object' && (action as { type?: string }).type === 'gallery/imageSelected' +/** + * Every write to the selection, in order and raw — so an expectation pins the whole payload *and* + * that there was exactly one write. Returning just the first match let a stray second dispatch + * (which is what the user would actually end up looking at) pass unnoticed. + * + * The two branches use different actions deliberately: advancing to a neighbour *picks* an item + * (`imageSelected`), while keeping the displayed item and dropping the deleted ones from the + * multi-selection is a *mutation* (`selectionChanged`), which the viewer does not treat as the + * user asking to see anything. + */ +const getSelectionWrites = (dispatched: unknown[]) => + dispatched.filter( + (candidate): candidate is { type: string; payload: string | string[] | null } => + !!candidate && + typeof candidate === 'object' && + ((candidate as { type?: string }).type === 'gallery/imageSelected' || + (candidate as { type?: string }).type === 'gallery/selectionChanged') ); describe('handleDeletions selection behavior', () => { @@ -103,17 +133,22 @@ describe('handleDeletions selection behavior', () => { // Before the fix this dispatched imageSelected(null) and dropped the viewer to its // empty state even though the displayed video still exists. - expect(getSelectionChange(dispatched)?.payload).toBe('c.mp4'); + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['c.mp4'] }]); }); - it('keeps viewing the displayed image on hover-delete of another selected image', async () => { + it('keeps viewing the displayed image on hover-delete of another selected image, without re-picking it', async () => { // Multi-selection [a, b] with b displayed; the hover delete button deletes only a. vi.mocked(selectLastSelectedItem).mockReturnValue('b.png'); const { store, dispatched } = buildStore(['a.png', 'b.png'], new Set()); await handleDeletions(['a.png'], store); - expect(getSelectionChange(dispatched)?.payload).toBe('b.png'); + // The action matters, not just the resulting state (PR #9520 review). `imageSelected('b.png')` + // would leave the very same selection, but it is the action that means "the user picked this", + // and the viewer answers a pick by lifting an in-progress generation's overlay off the item for + // two seconds. Deleting `a` must not flash `b` over a running render. + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['b.png'] }]); + expect(imageSelected).not.toHaveBeenCalled(); }); it('does not move the selection when the displayed image fails to delete', async () => { @@ -122,7 +157,7 @@ describe('handleDeletions selection behavior', () => { await handleDeletions(['a.png'], store); - expect(getSelectionChange(dispatched), 'a failed delete must not advance the selection').toBeUndefined(); + expect(getSelectionWrites(dispatched), 'a failed delete must not advance the selection').toEqual([]); }); it('keeps a surviving (failed-delete) neighbour as the replacement candidate', async () => { @@ -133,7 +168,7 @@ describe('handleDeletions selection behavior', () => { await handleDeletions(['a.png', 'b.png'], store); // If b.png were treated as deleted, the selection would skip to c.mp4. - expect(getSelectionChange(dispatched)?.payload).toBe('b.png'); + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/imageSelected', payload: 'b.png' }]); }); it('advances to the nearest surviving neighbour when everything requested is deleted', async () => { @@ -142,6 +177,44 @@ describe('handleDeletions selection behavior', () => { await handleDeletions(['b.png'], store); - expect(getSelectionChange(dispatched)?.payload).toBe('a.png'); + // The displayed item is gone, so the viewer really does move to a different image: that is a + // pick, and revealing it over a running generation is the point. + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/imageSelected', payload: 'a.png' }]); + }); + + it('keeps surviving siblings of a larger multi-selection', async () => { + // No race here: [a, b, c] with c displayed, delete a. Collapsing onto the displayed item used + // to deselect b as collateral, which the branch's own comment never claimed to do. + vi.mocked(selectLastSelectedItem).mockReturnValue('c.mp4'); + const { store, dispatched } = buildStore(['a.png', 'b.png', 'c.mp4'], new Set()); + + await handleDeletions(['a.png'], store); + + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['b.png', 'c.mp4'] }]); + }); + + it('leaves a selection made while the delete was in flight alone', async () => { + // The branch decides on a snapshot taken before the request, so by the time it runs the user + // may have selected something else. Collapsing onto the snapshot would discard that pick *and* + // move the active item back — which the viewer publishes as a change of active item and + // reveals, the very flash the mutation action avoids. + vi.mocked(selectLastSelectedItem).mockReturnValue('b.png'); + const { store, dispatched } = buildStore(['a.png', 'b.png'], new Set(), ['a.png', 'b.png', 'c.mp4']); + + await handleDeletions(['a.png'], store); + + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['b.png', 'c.mp4'] }]); + expect(imageSelected).not.toHaveBeenCalled(); + }); + + it('falls back to the surviving displayed item when the newer selection is all deleted', async () => { + // Same race, but everything the user selected meanwhile went away: there is nothing to keep, + // so the viewer stays on the item that survived rather than emptying out. + vi.mocked(selectLastSelectedItem).mockReturnValue('b.png'); + const { store, dispatched } = buildStore(['a.png', 'b.png'], new Set(), ['a.png']); + + await handleDeletions(['a.png'], store); + + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['b.png'] }]); }); }); diff --git a/invokeai/frontend/web/src/features/deleteImageModal/store/state.ts b/invokeai/frontend/web/src/features/deleteImageModal/store/state.ts index 8169a54bafc..fab65eb7156 100644 --- a/invokeai/frontend/web/src/features/deleteImageModal/store/state.ts +++ b/invokeai/frontend/web/src/features/deleteImageModal/store/state.ts @@ -12,7 +12,7 @@ import { selectCanvasSlice } from 'features/controlLayers/store/selectors'; import type { CanvasState, RefImagesState } from 'features/controlLayers/store/types'; import type { ImageUsage } from 'features/deleteImageModal/store/types'; import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; -import { imageSelected } from 'features/gallery/store/gallerySlice'; +import { imageSelected, selectionChanged } from 'features/gallery/store/gallerySlice'; import { pickSelectionAfterDelete, selectCachedGalleryItemNames, @@ -107,7 +107,26 @@ export const handleDeletions = async (image_names: string[], store: AppStore) => // video displayed while images were deleted, or a hover-delete of another item) // or its own delete failed. Keep viewing it and just prune the deleted items // from the multi-selection. - dispatch(imageSelected(lastSelected)); + // + // This is a mutation, not a pick. `imageSelected` would leave the same state in the + // ordinary case, but it is the action that means "the user asked to see this" — and + // while a generation is running the viewer answers that by lifting the progress + // overlay off the item for a couple of seconds. Deleting some *other* item is not a + // request to look at the one already on screen. See gallerySelectionSource. + // + // Filtered from the *live* selection rather than collapsed onto the pre-await + // snapshot: the user can select something else while the delete is in flight, and + // collapsing would both discard that and move the active item — which publishes as + // a change of active item and reveals anyway, the very flash this avoids. + // + // The fallback (everything they had selected meanwhile was deleted) does not get + // that guarantee: `lastSelected` is not in the live selection at all there, so it + // moves the active item and does reveal — and `deletedNames` only proves it outlived + // *this* request, so a second delete overlapping this one can leave it pointing at + // an item that is already gone. Both are pre-existing and want the advance branch's + // neighbour search, which cannot run off these pre-await snapshots. + const survivors = getState().gallery.selection.filter((name) => !deletedNames.has(name)); + dispatch(selectionChanged(survivors.length > 0 ? survivors : [lastSelected])); } else { // Advance to a still-living neighbour (prev > next) so the Viewer keeps a real // selection. May pick a video — the polymorphic list intentionally allows that. diff --git a/invokeai/frontend/web/src/features/deleteVideoModal/store/state.test.ts b/invokeai/frontend/web/src/features/deleteVideoModal/store/state.test.ts index 82fdfeb3ff2..f1fb3b4f0ec 100644 --- a/invokeai/frontend/web/src/features/deleteVideoModal/store/state.test.ts +++ b/invokeai/frontend/web/src/features/deleteVideoModal/store/state.test.ts @@ -36,6 +36,7 @@ vi.mock('features/gallery/store/gallerySelectors', () => ({ vi.mock('features/gallery/store/gallerySlice', () => ({ imageSelected: vi.fn((payload: string | null) => ({ type: 'gallery/imageSelected', payload })), + selectionChanged: vi.fn((payload: string[]) => ({ type: 'gallery/selectionChanged', payload })), })); vi.mock('features/nodes/store/nodesSlice', () => ({ @@ -75,33 +76,67 @@ const buildVideoFieldNode = (nodeId: string, videoName: string) => ({ }, }); -const buildStore = (selection: string[], failingNames: Set, nodes: unknown[] = [], rejectAll = false) => { +/** + * `selectionDuringDelete` re-points the store's selection when the delete request is issued, + * standing in for the user selecting something else while it is in flight. The rest of + * handleDeletions then runs against that newer selection, as it does in the app. + * + * It writes `currentSelection` directly rather than dispatching, so the simulated gesture does not + * land in `dispatched` and cannot be mistaken for the production write under test. The seam is + * only equivalent to a real mid-flight click because neither modal reads state between issuing the + * request and the post-await block; anything added in between would need a real dispatch here. + */ +const buildStore = ( + selection: string[], + failingNames: Set, + nodes: unknown[] = [], + rejectAll = false, + selectionDuringDelete?: string[] +) => { const dispatched: unknown[] = []; + let currentSelection = selection; const dispatch = vi.fn((action: unknown) => { dispatched.push(action); const typed = action as { type?: string; video_names?: string[] }; if (typed?.type === 'videosApi/deleteVideos') { return { - unwrap: () => - rejectAll + unwrap: () => { + if (selectionDuringDelete) { + currentSelection = selectionDuringDelete; + } + return rejectAll ? Promise.reject(new Error('delete failed')) : Promise.resolve({ deleted_videos: (typed.video_names ?? []).filter((name) => !failingNames.has(name)), failed_videos: (typed.video_names ?? []).filter((name) => failingNames.has(name)), affected_boards: ['none'], - }), + }); + }, }; } return action; }); - const getState = vi.fn(() => ({ gallery: { selection }, nodes: { present: { nodes } } })); + const getState = vi.fn(() => ({ gallery: { selection: currentSelection }, nodes: { present: { nodes } } })); return { store: { dispatch, getState } as unknown as AppStore, dispatched }; }; -const getSelectionChange = (dispatched: unknown[]) => - dispatched.find( - (action): action is { type: string; payload: string | null } => - !!action && typeof action === 'object' && (action as { type?: string }).type === 'gallery/imageSelected' +/** + * Every write to the selection, in order and raw — so an expectation pins the whole payload *and* + * that there was exactly one write. Returning just the first match let a stray second dispatch + * (which is what the user would actually end up looking at) pass unnoticed. + * + * The two branches use different actions deliberately: advancing to a neighbour *picks* an item + * (`imageSelected`), while keeping the displayed item and dropping the deleted ones from the + * multi-selection is a *mutation* (`selectionChanged`), which the viewer does not treat as the + * user asking to see anything. + */ +const getSelectionWrites = (dispatched: unknown[]) => + dispatched.filter( + (candidate): candidate is { type: string; payload: string | string[] | null } => + !!candidate && + typeof candidate === 'object' && + ((candidate as { type?: string }).type === 'gallery/imageSelected' || + (candidate as { type?: string }).type === 'gallery/selectionChanged') ); const getVideoFieldChanges = (dispatched: unknown[]) => @@ -143,7 +178,7 @@ describe('handleDeletions selection behavior on partial failure', () => { await handleDeletions(['a.mp4'], store); - expect(getSelectionChange(dispatched), 'a failed delete must not advance the selection').toBeUndefined(); + expect(getSelectionWrites(dispatched), 'a failed delete must not advance the selection').toEqual([]); }); it('does not move the selection when the whole batch request fails', async () => { @@ -152,7 +187,7 @@ describe('handleDeletions selection behavior on partial failure', () => { await handleDeletions(['a.mp4'], store); - expect(getSelectionChange(dispatched)).toBeUndefined(); + expect(getSelectionWrites(dispatched)).toEqual([]); }); it('keeps a surviving (failed-delete) neighbour as the replacement candidate', async () => { @@ -163,11 +198,11 @@ describe('handleDeletions selection behavior on partial failure', () => { await handleDeletions(['a.mp4', 'b.mp4'], store); // Before the fix, b.mp4 was excluded as "deleted" and the selection skipped to c.png. - expect(getSelectionChange(dispatched)?.payload).toBe('b.mp4'); + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/imageSelected', payload: 'b.mp4' }]); expect(toast).toHaveBeenCalledWith(expect.objectContaining({ status: 'warning' })); }); - it('keeps viewing the displayed video when its delete fails but another selected video was deleted', async () => { + it('keeps viewing the displayed video when another selected video was deleted, without re-picking it', async () => { vi.mocked(selectLastSelectedItem).mockReturnValue('a.mp4'); const { store, dispatched } = buildStore(['a.mp4', 'b.mp4'], new Set(['a.mp4'])); @@ -175,7 +210,13 @@ describe('handleDeletions selection behavior on partial failure', () => { // The multi-selection contained a deleted item (b), so the selection is pruned — but it // must land on the still-existing displayed video, not jump to a neighbour. - expect(getSelectionChange(dispatched)?.payload).toBe('a.mp4'); + // + // And it must prune rather than re-pick (PR #9520 review): `imageSelected('a.mp4')` leaves the + // very same selection, but it is the action that means "the user picked this", which makes the + // viewer lift an in-progress generation's overlay off the video for two seconds. Deleting some + // other video is not a request to look at this one. + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['a.mp4'] }]); + expect(imageSelected).not.toHaveBeenCalled(); }); it('advances to the nearest surviving neighbour when everything requested is deleted', async () => { @@ -184,9 +225,41 @@ describe('handleDeletions selection behavior on partial failure', () => { await handleDeletions(['b.mp4'], store); - expect(getSelectionChange(dispatched)?.payload).toBe('a.mp4'); + // The displayed item is gone, so the viewer really does move to a different video: that is a + // pick, and revealing it over a running generation is the point. + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/imageSelected', payload: 'a.mp4' }]); expect(imageSelected).toHaveBeenCalledWith('a.mp4'); }); + + it('leaves a selection made while the delete was in flight alone', async () => { + // The branch decides on a snapshot taken before the request, so by the time it runs the user + // may have selected something else. Collapsing onto the snapshot would discard that pick *and* + // move the active item back — which the viewer publishes as a change of active item and + // reveals, the very flash the mutation action avoids. + vi.mocked(selectLastSelectedItem).mockReturnValue('a.mp4'); + const { store, dispatched } = buildStore(['a.mp4', 'b.mp4'], new Set(['a.mp4']), [], false, [ + 'a.mp4', + 'b.mp4', + 'c.png', + ]); + + await handleDeletions(['a.mp4', 'b.mp4'], store); + + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['a.mp4', 'c.png'] }]); + expect(imageSelected).not.toHaveBeenCalled(); + }); + + it('falls back to the surviving displayed video when the newer selection is all deleted', async () => { + // Same race, but everything the user selected meanwhile went away. Without the fallback this + // dispatches selectionChanged([]) and drops the viewer to its empty-state placeholder while a + // surviving video is still on screen — the original #9163 bug this file exists to guard. + vi.mocked(selectLastSelectedItem).mockReturnValue('a.mp4'); + const { store, dispatched } = buildStore(['a.mp4'], new Set(['a.mp4']), [], false, ['b.mp4']); + + await handleDeletions(['a.mp4', 'b.mp4'], store); + + expect(getSelectionWrites(dispatched)).toEqual([{ type: 'gallery/selectionChanged', payload: ['a.mp4'] }]); + }); }); describe('handleDeletions node VideoField cleanup', () => { diff --git a/invokeai/frontend/web/src/features/deleteVideoModal/store/state.ts b/invokeai/frontend/web/src/features/deleteVideoModal/store/state.ts index 9821267088e..96227b96909 100644 --- a/invokeai/frontend/web/src/features/deleteVideoModal/store/state.ts +++ b/invokeai/frontend/web/src/features/deleteVideoModal/store/state.ts @@ -3,7 +3,7 @@ import type { AppDispatch, AppStore, RootState } from 'app/store/store'; import { useAppStore } from 'app/store/storeHooks'; import { intersection } from 'es-toolkit/compat'; import { selectLastSelectedItem } from 'features/gallery/store/gallerySelectors'; -import { imageSelected } from 'features/gallery/store/gallerySlice'; +import { imageSelected, selectionChanged } from 'features/gallery/store/gallerySlice'; import { pickSelectionAfterDelete, selectCachedGalleryItemNames, @@ -127,7 +127,26 @@ export const handleDeletions = async (video_names: string[], store: AppStore) => if (lastSelected && !deletedNames.has(lastSelected)) { // The displayed item survived (its delete failed) — keep viewing it and just prune // the deleted items from the multi-selection. - dispatch(imageSelected(lastSelected)); + // + // This is a mutation, not a pick. `imageSelected` would leave the same state in the + // ordinary case, but it is the action that means "the user asked to see this" — and + // while a generation is running the viewer answers that by lifting the progress + // overlay off the item for a couple of seconds. Deleting some *other* item is not a + // request to look at the one already on screen. See gallerySelectionSource. + // + // Filtered from the *live* selection rather than collapsed onto the pre-await + // snapshot: the user can select something else while the delete is in flight, and + // collapsing would both discard that and move the active item — which publishes as a + // change of active item and reveals anyway, the very flash this avoids. + // + // The fallback (everything they had selected meanwhile was deleted) does not get that + // guarantee: `lastSelected` is not in the live selection at all there, so it moves the + // active item and does reveal — and `deletedNames` only proves it outlived *this* + // request, so a second delete overlapping this one can leave it pointing at an item + // that is already gone. Both are pre-existing and want the advance branch's neighbour + // search, which cannot run off these pre-await snapshots. + const survivors = stateAfter.gallery.selection.filter((name) => !deletedNames.has(name)); + dispatch(selectionChanged(survivors.length > 0 ? survivors : [lastSelected])); } else { const replacement = lastSelectedIndex >= 0 ? pickSelectionAfterDelete(galleryItemNames, lastSelectedIndex, deletedNames) : null; diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryImage.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryImage.tsx index baf88e1bb2e..5ef80e914e3 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryImage.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryImage.tsx @@ -17,7 +17,12 @@ import { dndInputFix } from 'features/dnd/util'; import { useImageContextMenu } from 'features/gallery/components/ContextMenu/ImageContextMenu'; import { GalleryItemHoverIcons } from 'features/gallery/components/ImageGrid/GalleryItemHoverIcons'; import { selectSelectedBoardId, selectSelection } from 'features/gallery/store/gallerySelectors'; -import { imageToCompareChanged, selectGallerySlice, selectionChanged } from 'features/gallery/store/gallerySlice'; +import { + imageSelected, + imageToCompareChanged, + selectGallerySlice, + selectionChanged, +} from 'features/gallery/store/gallerySlice'; import { selectCachedGalleryItemNames } from 'features/gallery/store/selectCachedGalleryItemNames'; import { isVideoName } from 'features/gallery/store/types'; import { navigationApi } from 'features/ui/layouts/navigation-api'; @@ -46,7 +51,7 @@ const buildOnClick = if (itemNames.length === 0) { // Without an ordered list we can still honor a plain single-click. if (!shiftKey && !ctrlKey && !metaKey && !altKey) { - dispatch(selectionChanged([imageName])); + dispatch(imageSelected(imageName)); } return; } @@ -80,7 +85,12 @@ const buildOnClick = dispatch(selectionChanged(uniq(selection.concat(imageName)))); } } else { - dispatch(selectionChanged([imageName])); + // A plain click is the user picking one item, where the modifier branches above mutate the + // multi-selection. The distinction is load-bearing: the gallery selection source reads + // `imageSelected` as a pick (so a repeat click on the active item reveals it over the + // progress overlay) but deliberately does not read `selectionChanged`, whose mutations only + // count when they move the active item. + dispatch(imageSelected(imageName)); } }; diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryVideoItem.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryVideoItem.tsx index b6af3ff149c..3bdf4b2db6f 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryVideoItem.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageGrid/GalleryVideoItem.tsx @@ -14,7 +14,7 @@ import { selectSelectedBoardId, selectSelection, } from 'features/gallery/store/gallerySelectors'; -import { selectGallerySlice, selectionChanged } from 'features/gallery/store/gallerySlice'; +import { imageSelected, selectGallerySlice, selectionChanged } from 'features/gallery/store/gallerySlice'; import { selectCachedGalleryItemNames } from 'features/gallery/store/selectCachedGalleryItemNames'; import { isVideoName } from 'features/gallery/store/types'; import { navigationApi } from 'features/ui/layouts/navigation-api'; @@ -52,7 +52,7 @@ const buildOnClick = if (itemNames.length === 0) { // Without an ordered list, only basic single-click selection is possible. if (!shiftKey && !ctrlKey && !metaKey && !altKey) { - dispatch(selectionChanged([videoName])); + dispatch(imageSelected(videoName)); } return; } @@ -61,7 +61,7 @@ const buildOnClick = if (altKey) { // Alt-click is image-only (comparison view). Quietly treat as a normal click for videos. - dispatch(selectionChanged([videoName])); + dispatch(imageSelected(videoName)); } else if (shiftKey) { const lastSelectedItem = selection.at(-1); const lastClickedIndex = itemNames.findIndex((name) => name === lastSelectedItem); @@ -82,7 +82,9 @@ const buildOnClick = dispatch(selectionChanged(uniq(selection.concat(videoName)))); } } else { - dispatch(selectionChanged([videoName])); + // A plain click is a pick of one item, not a mutation of the multi-selection — see the same + // branch in GalleryImage for why the gallery selection source depends on the distinction. + dispatch(imageSelected(videoName)); } }; 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 ca0271d6f76..c37831be936 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx @@ -7,7 +7,6 @@ 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'; @@ -21,7 +20,7 @@ import { AnimatePresence, motion } from 'framer-motion'; import { memo, useCallback, useEffect, useRef, useState } from 'react'; import type { ImageDTO } from 'services/api/types'; -import { SELECTED_ITEM_MEDIA_GRACE_MS, SELECTED_ITEM_REVEAL_DURATION_MS, useImageViewerContext } from './context'; +import { useImageViewerContext } from './context'; import { NoContentForViewer } from './NoContentForViewer'; import { ProgressImage } from './ProgressImage2'; import { ProgressImageTiles } from './ProgressImageTiles'; @@ -41,7 +40,7 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu $activeProgressData, $isProgressImageResolving, $isTemporarilyShowingSelectedImage, - lastRenderedItemNameRef, + revealMachine, } = useImageViewerContext(); const progressEvent = useStore($progressEvent); const progressImage = useStore($progressImage); @@ -119,14 +118,9 @@ export const CurrentImagePreview = memo(({ imageDTO }: { imageDTO: ImageDTO | nu // around it lives in the hook, where it is mounted and tested with real lifecycles. The image // path only renders an image once its preload has settled, so whatever is rendered has painted. useSelectedItemReveal({ - lastRenderedItemNameRef, - $isTemporarilyShowingSelectedImage, - marker: autoSwitchedImages, - durationMs: SELECTED_ITEM_REVEAL_DURATION_MS, - mediaGraceMs: SELECTED_ITEM_MEDIA_GRACE_MS, + revealMachine, renderedItemName: imageToRender?.image_name ?? null, isMediaReady: imageToRender !== null, - selectedItemName: selectedImageName ?? null, shouldShowProgressInViewer, hasProgressImage, isProgressImageResolving, diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.test.ts index 1a79715f614..d48e950ee95 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.test.ts @@ -21,7 +21,9 @@ describe('CurrentVideoPreview progress overlay', () => { expect(source).toMatch( /withProgress =\s+shouldShowProgressInViewer && hasProgressImage && !isTemporarilyShowingSelectedImage && !isPlaying/ ); - expect(source).toContain('SELECTED_ITEM_REVEAL_DURATION_MS'); + // The reveal's timing constants live with the machine in the viewer context now; what this + // component owns is consulting the shared atom in its overlay gate, asserted above. + expect(source).toContain('revealMachine,'); }); it('tiles concurrent sessions instead of letting them overwrite each other (multi-GPU)', () => { diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx index 87c991083e5..92dc9c897d3 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentVideoPreview.tsx @@ -13,12 +13,7 @@ import { dndInputFix } from 'features/dnd/util'; import VideoMetadataViewer from 'features/gallery/components/ImageMetadataViewer/VideoMetadataViewer'; import NextPrevItemButtons from 'features/gallery/components/NextPrevItemButtons'; import { useNextPrevItemNavigation } from 'features/gallery/components/useNextPrevItemNavigation'; -import { autoSwitchedImages } from 'features/gallery/store/autoSwitchedImages'; -import { - selectLastSelectedItem, - selectSelectedBoardId, - selectSelection, -} from 'features/gallery/store/gallerySelectors'; +import { selectSelectedBoardId, selectSelection } from 'features/gallery/store/gallerySelectors'; import { isVideoName } from 'features/gallery/store/types'; import { useRegisteredHotkeys } from 'features/system/components/HotkeysModal/useHotkeyData'; import { toast } from 'features/toast/toast'; @@ -35,7 +30,7 @@ import { useTranslation } from 'react-i18next'; import { PiArrowSquareOutBold, PiCopyBold, PiDownloadSimpleBold, PiTrashSimpleBold, PiXBold } from 'react-icons/pi'; import type { VideoDTO } from 'services/api/types'; -import { SELECTED_ITEM_MEDIA_GRACE_MS, SELECTED_ITEM_REVEAL_DURATION_MS, useImageViewerContext } from './context'; +import { useImageViewerContext } from './context'; import { NoContentForViewer } from './NoContentForViewer'; import { ProgressImage } from './ProgressImage2'; import { ProgressImageTiles } from './ProgressImageTiles'; @@ -87,7 +82,7 @@ export const CurrentVideoPreview = memo(({ videoDTO }: Props) => { $activeProgressData, $isProgressImageResolving, $isTemporarilyShowingSelectedImage, - lastRenderedItemNameRef, + revealMachine, onLoadImage, } = useImageViewerContext(); const progressEvent = useStore($progressEvent); @@ -106,7 +101,6 @@ export const CurrentVideoPreview = memo(({ videoDTO }: Props) => { // of letting the sessions overwrite each other's full-size preview. Mirrors CurrentImagePreview. const withTiledProgress = withProgress && activeProgressData.length > 1; const { goToPreviousImage, goToNextImage, isFetching } = useNextPrevItemNavigation(); - const selectedItemName = useAppSelector(selectLastSelectedItem); // One controller per mounted preview component; the previous-item ref inside it is the shared // one from the viewer context, so image <-> video clicks read as selection changes on both ends. @@ -122,14 +116,9 @@ export const CurrentVideoPreview = memo(({ videoDTO }: Props) => { // mounted and tested with real lifecycles. preload="metadata" plus the near-zero seek does not // prove a frame exists, so readiness comes from usePaintedItemName fed by onLoadedData. useSelectedItemReveal({ - lastRenderedItemNameRef, - $isTemporarilyShowingSelectedImage, - marker: autoSwitchedImages, - durationMs: SELECTED_ITEM_REVEAL_DURATION_MS, - mediaGraceMs: SELECTED_ITEM_MEDIA_GRACE_MS, + revealMachine, renderedItemName: videoName, - isMediaReady, - selectedItemName: selectedItemName ?? null, + isMediaReady: isMediaReady, shouldShowProgressInViewer, hasProgressImage, isProgressImageResolving, 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 a92558b4453..f7fc8ca6257 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx @@ -7,18 +7,22 @@ import type { ViewerProgressDatum, } from 'features/gallery/components/ImageViewer/viewerProgressLifecycle'; import { createViewerProgressLifecycle } from 'features/gallery/components/ImageViewer/viewerProgressLifecycle'; +import { $gallerySelection } from 'features/gallery/store/gallerySelectionSource'; import { selectAutoSwitch } from 'features/gallery/store/gallerySelectors'; import type { ProgressImage as ProgressImageType } from 'features/nodes/types/common'; import { LRUCache } from 'lru-cache'; import { type Atom, atom, computed, map, type MapStore, type WritableAtom } from 'nanostores'; -import type { MutableRefObject, PropsWithChildren } from 'react'; -import { createContext, memo, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; +import type { PropsWithChildren } 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 type { SelectedItemRevealMachine } from './selectedItemReveal'; +import { createSelectedItemRevealMachine } from './selectedItemReveal'; + export type { ViewerProgressDatum } from 'features/gallery/components/ImageViewer/viewerProgressLifecycle'; type ImageViewerContextValue = { @@ -31,11 +35,9 @@ type ImageViewerContextValue = { $activeProgressData: Atom; $isProgressImageResolving: Atom; $isTemporarilyShowingSelectedImage: WritableAtom; - /** Name of the item most recently rendered by either preview component (image or video). Shared - * across the two components so a click that switches media type (image -> video or back) still - * reads as a selection change to the temporary-reveal logic — a per-component ref resets on the - * swap and would silently swallow the first reveal after every type switch. */ - lastRenderedItemNameRef: MutableRefObject; + /** The viewer's reveal state machine. Both preview components drive this one instance, so a + * click that switches media type is just another selection. */ + revealMachine: SelectedItemRevealMachine; /** * The viewer finished loading the final image/video for the given session (its DTO's * `session_id`, or null when it has none). Ends the completed session's "resolve" illusion. @@ -44,12 +46,12 @@ type ImageViewerContextValue = { }; /** How long a mid-generation gallery click shows the clicked item before the live preview returns. */ -export const SELECTED_ITEM_REVEAL_DURATION_MS = 2000; +const SELECTED_ITEM_REVEAL_DURATION_MS = 2000; -/** How long a reveal waits for the selected item's first frame before showing it anyway. Long - * enough for a decode on a slow connection, short enough that a click on media that will never - * load still lands well inside the reveal it was promised. */ -export const SELECTED_ITEM_MEDIA_GRACE_MS = 1000; +/** How long a reveal waits for the selected item's media to paint before showing it anyway. Long + * enough for a decode or a first video frame on a slow connection, short enough that a click on + * media that will never load still lands well inside the reveal it was promised. */ +const SELECTED_ITEM_MEDIA_GRACE_MS = 1000; const ImageViewerContext = createContext(null); @@ -73,7 +75,13 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { )[0]; const $isProgressImageResolving = useState(() => atom(false))[0]; const $isTemporarilyShowingSelectedImage = useState(() => atom(false))[0]; - const lastRenderedItemNameRef = useRef(null); + const [revealMachine] = useState(() => + createSelectedItemRevealMachine({ + setRevealed: (revealed) => $isTemporarilyShowingSelectedImage.set(revealed), + durationMs: SELECTED_ITEM_REVEAL_DURATION_MS, + mediaGraceMs: SELECTED_ITEM_MEDIA_GRACE_MS, + }) + ); // 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 })); @@ -93,6 +101,14 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { }) )[0]; + // Selections that land while no preview component is mounted (comparison mode) have to be + // settled somewhere, or returning to the viewer replays them as a reveal nobody asked for. + useEffect(() => $gallerySelection.listen((selection) => revealMachine.noteSelection(selection)), [revealMachine]); + + // The machine outlives the components that drive it, so its timers are this provider's to clean + // up: without this an armed grace timer fires on a dead machine and raises an orphaned flag. + useEffect(() => () => revealMachine.reset(), [revealMachine]); + useEffect(() => { if (!socket) { return; @@ -216,7 +232,7 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { $activeProgressData, $isProgressImageResolving, $isTemporarilyShowingSelectedImage, - lastRenderedItemNameRef, + revealMachine, onLoadImage, }), [ @@ -228,6 +244,7 @@ export const ImageViewerContextProvider = memo((props: PropsWithChildren) => { $progressEvent, $progressImage, onLoadImage, + revealMachine, ] ); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts index 1216869a7e1..8642fa127a4 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.test.ts @@ -1,31 +1,28 @@ -import { createAutoSwitchedSelectionMarker } from 'features/gallery/store/autoSwitchedImages'; +import type { GallerySelectionDescriptor } from 'features/gallery/store/gallerySelectionSource'; import { describe, expect, it } from 'vitest'; -import { createSelectedItemRevealController } from './selectedItemReveal'; +import { createSelectedItemRevealMachine } from './selectedItemReveal'; -/** - * Drives the controller through the same call sequences the preview components' effects produce, - * with a hand-cranked timer so reveal expiry is explicit. The marker is the real implementation — - * the reveal/auto-switch interplay is exactly what these tests exist to pin. - */ +const DURATION_MS = 2000; +const MEDIA_GRACE_MS = 1000; + +/** Drives the machine the way the preview components' effects do, with hand-cranked timers. */ const createHarness = () => { let revealed = false; - const lastRenderedItemNameRef = { current: null as string | null }; - const marker = createAutoSwitchedSelectionMarker(); - const timers = new Map void>(); + let generation = 0; + let selection: GallerySelectionDescriptor = { name: null, generation: 0, isAutoSwitch: false }; + const timers = new Map void; ms: number }>(); let nextTimerId = 1; - const controller = createSelectedItemRevealController({ - lastRenderedItemNameRef, - marker, + const machine = createSelectedItemRevealMachine({ setRevealed: (value) => { revealed = value; }, - durationMs: 2000, - mediaGraceMs: 1000, - schedule: (fn) => { + durationMs: DURATION_MS, + mediaGraceMs: MEDIA_GRACE_MS, + schedule: (fn, ms) => { const id = nextTimerId++; - timers.set(id, fn); + timers.set(id, { fn, ms }); return id; }, cancel: (id) => { @@ -33,305 +30,273 @@ const createHarness = () => { }, }); + const base = { + renderedItemName: null as string | null, + isMediaReady: false, + shouldShowProgressInViewer: true, + hasProgressImage: true, + isProgressImageResolving: false, + }; + return { - controller, - marker, - lastRenderedItemNameRef, + machine, isRevealed: () => revealed, - // The components' unmount handler lowers the shared flag directly, without going through the - // controller — StrictMode runs it between the doubled mount effects. - lowerExternally: () => { - revealed = false; + /** The provider's $gallerySelection subscription. */ + noteSelection: () => machine.noteSelection(selection), + pendingTimerCount: () => timers.size, + /** A selection dispatch, as the gallery source listener would publish it. */ + select: (name: string | null, options: { isAutoSwitch?: boolean } = {}) => { + generation += 1; + selection = { name, generation, isAutoSwitch: options.isAutoSwitch ?? false }; + }, + /** One effect run. */ + sync: (overrides: Partial = {}) => { + machine.sync({ selection, ...base, ...overrides }); + }, + /** The viewer showing an item whose media has painted. */ + syncRendered: (itemName: string, overrides: Partial = {}) => { + machine.sync({ selection, ...base, renderedItemName: itemName, isMediaReady: true, ...overrides }); }, fireTimers: () => { - for (const [id, fn] of [...timers]) { + for (const [id, timer] of [...timers]) { timers.delete(id); - fn(); + timer.fn(); } }, - pendingTimerCount: () => timers.size, }; }; -// An auto-switch as onInvocationComplete + the selection listener produce it: record, then the -// dispatched selection lands and settles the marker. -const autoSwitchTo = (marker: ReturnType, itemName: string) => { - marker.record(itemName); - marker.settle(itemName); -}; - -const inputs = (overrides: Partial['controller']['run']>[0]> = {}) => ({ - shouldShowProgressInViewer: true, - hasProgressImage: true, - isProgressImageResolving: false, - renderedItemName: null as string | null, - selectedItemName: null as string | null, - // Most sequences are about *which* item is revealed, not about waiting for pixels, so the - // default is "the item on screen has painted". The media-hold tests override it. - isMediaReady: true, - ...overrides, -}); - -const rendering = (itemName: string) => inputs({ renderedItemName: itemName, selectedItemName: itemName }); - -describe('createSelectedItemRevealController', () => { - it('reveals a mid-render selection change, then lowers when the timer fires', () => { +describe('createSelectedItemRevealMachine', () => { + it('reveals a mid-render click once its media has painted, then lowers on the timer', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); + h.select('b.png'); + h.sync(); + expect(h.isRevealed(), 'not before the media is ready').toBe(false); + + h.syncRendered('b.png'); expect(h.isRevealed()).toBe(true); + h.fireTimers(); expect(h.isRevealed()).toBe(false); }); - it('does not reveal the first render after the viewer opens', () => { + it('does not lift the overlay onto an element that has not painted yet', () => { + // The video element mounts immediately but shows black until it decodes a frame; revealing + // then would replace the live preview with a black rectangle. const h = createHarness(); - h.controller.run(rendering('a.png')); + h.select('b.mp4'); + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false }); expect(h.isRevealed()).toBe(false); }); - it('does not re-reveal when a later run finds the same item and no reveal in flight', () => { + it('reveals anyway when the media never becomes ready', () => { + // A failed load or an undecodable codec must not swallow the click for the whole render. const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); + h.select('broken.mp4'); + h.sync({ renderedItemName: 'broken.mp4', isMediaReady: false }); h.fireTimers(); - // e.g. the resolving flag flapping, or any other dependency change with the item unchanged. - h.controller.run(rendering('b.png')); - expect(h.isRevealed()).toBe(false); + expect(h.isRevealed()).toBe(true); }); - it('does not reveal an auto-switched item', () => { + it('does not run the grace against an item that has not rendered yet', () => { + // A slow DTO fetch leaves the component rendering the previous item. Counting the grace from + // the click would lift the overlay onto that stale content at the deadline — and then leave + // the real item covered when it finally arrived, the reveal already spent. const h = createHarness(); - h.controller.run(rendering('a.png')); - autoSwitchTo(h.marker, 'b.png'); - h.controller.run(rendering('b.png')); + h.select('slow.png'); + h.sync({ renderedItemName: 'previous.png', isMediaReady: true }); + expect(h.pendingTimerCount(), 'no deadline while the item has nothing to show').toBe(0); + h.fireTimers(); expect(h.isRevealed()).toBe(false); - }); - it('consumes the marker even when no progress is showing, so a later click on that item reveals', () => { - const h = createHarness(); - h.controller.run(rendering('a.png')); - autoSwitchTo(h.marker, 'b.png'); - // The auto-switched item renders with the overlay down — the common case. - h.controller.run(inputs({ renderedItemName: 'b.png', selectedItemName: 'b.png', hasProgressImage: false })); - expect(h.isRevealed()).toBe(false); - // A new render starts; the user clicks away and back to the once-auto-switched item. - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); - expect(h.isRevealed()).toBe(true); + // The item lands long after any selection-anchored deadline would have expired. + h.syncRendered('slow.png'); + expect(h.isRevealed(), 'the claim is still owed when the item finally renders').toBe(true); }); - it('keeps the reveal through a StrictMode double-invoked mount effect', () => { - // React StrictMode mounts run effect -> cleanup -> effect. The cleanup cancels the reveal - // timer and the unmount handler lowers the shared flag; the second run then finds the shared - // ref already holding the new name. Without the re-arm, every cross-media first reveal dies - // in development. + it('starts the media grace only once the item is rendered, and only once', () => { const h = createHarness(); - h.controller.run(rendering('previous-image.png')); - h.controller.run(rendering('clicked-video.mp4')); - expect(h.isRevealed()).toBe(true); - h.controller.clearTimer(); - h.lowerExternally(); - h.controller.run(rendering('clicked-video.mp4')); - expect(h.isRevealed()).toBe(true); + h.select('b.mp4'); + h.sync(); + expect(h.pendingTimerCount()).toBe(0); + + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false }); + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false }); expect(h.pendingTimerCount()).toBe(1); h.fireTimers(); - expect(h.isRevealed()).toBe(false); + expect(h.isRevealed(), 'a frame that never paints still cannot swallow the click').toBe(true); }); - it('reveals a click that landed inside a resolve window once the window ends', () => { + it('re-arms the grace when a hand-off interrupts the wait for a frame', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - // The user clicks a video while a finished render's preview is resolving; the next render's - // progress then resumes before the resolve ends. - h.controller.run(inputs({ isProgressImageResolving: true, renderedItemName: 'b.mp4', selectedItemName: 'b.mp4' })); - expect(h.isRevealed()).toBe(false); - // The ref must not have advanced past the click — that is what kept this click dead before. - expect(h.lastRenderedItemNameRef.current).toBe('a.png'); - h.controller.run(rendering('b.mp4')); + h.select('b.mp4'); + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false }); + expect(h.pendingTimerCount()).toBe(1); + + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false, isProgressImageResolving: true }); + expect(h.pendingTimerCount(), 'the hand-off owns the viewer; the deadline pauses with it').toBe(0); + + h.sync({ renderedItemName: 'b.mp4', isMediaReady: false }); + h.fireTimers(); expect(h.isRevealed()).toBe(true); }); - it('still suppresses an auto-switch whose render landed inside a resolve window', () => { - // The marker must not be consumed by a run that cannot reveal, or the post-resolve run would - // read the auto-switch as a user click. + it('never reveals an auto-switch', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - autoSwitchTo(h.marker, 'b.mp4'); - h.controller.run(inputs({ isProgressImageResolving: true, renderedItemName: 'b.mp4', selectedItemName: 'b.mp4' })); - expect(h.isRevealed()).toBe(false); - h.controller.run(rendering('b.mp4')); + h.select('finished.png', { isAutoSwitch: true }); + h.syncRendered('finished.png'); expect(h.isRevealed()).toBe(false); }); - it('reveals the selection made after the selection was cleared — including the same item', () => { + it('reveals the item already on screen when the user picks it again', () => { + // Nothing about the rendered item changes, so the previous-name comparison this replaces could + // not see it — the click was simply dead. const h = createHarness(); - h.controller.run(rendering('a.png')); - // Clearing the selection empties the viewer; the ref must not fall back to the "nothing has - // rendered yet" state or the next click is treated as the viewer's first render and stays - // hidden under the progress overlay. - h.controller.run(inputs({ renderedItemName: null, selectedItemName: null })); - h.controller.run(rendering('b.mp4')); - expect(h.isRevealed()).toBe(true); + h.select('a.png'); + h.syncRendered('a.png'); + h.fireTimers(); + expect(h.isRevealed()).toBe(false); - const h2 = createHarness(); - h2.controller.run(rendering('a.png')); - h2.controller.run(inputs({ renderedItemName: null, selectedItemName: null })); - h2.controller.run(rendering('a.png')); - expect(h2.isRevealed()).toBe(true); + h.select('a.png'); + h.syncRendered('a.png'); + expect(h.isRevealed()).toBe(true); }); - it('keeps the previous item while a selection exists but its render has not landed', () => { - // Image preloads and DTO fetches make the rendered item lag the selection; the in-between run - // must neither reveal nor erase the fact of what was on screen before. + it('does not reveal when the viewer simply opens on an existing selection', () => { + // No selection was dispatched, so nothing is owed a reveal. const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(inputs({ renderedItemName: null, selectedItemName: 'b.png' })); + h.syncRendered('a.png'); expect(h.isRevealed()).toBe(false); - expect(h.lastRenderedItemNameRef.current).toBe('a.png'); - h.controller.run(rendering('b.png')); - expect(h.isRevealed()).toBe(true); }); - it('does not reveal while the rendered item lags the selection', () => { + it('does not reveal a selection made while no progress was showing', () => { + // The click was already visible; a later generation starting must not flash it. const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(inputs({ renderedItemName: 'a.png', selectedItemName: 'b.png' })); + h.select('b.png'); + h.sync({ hasProgressImage: false }); + h.syncRendered('b.png'); expect(h.isRevealed()).toBe(false); }); - it('lowers the reveal when the overlay is not showing at all', () => { + it('holds a reveal owed during a resolve window until the window ends', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(inputs({ renderedItemName: 'b.png', selectedItemName: 'b.png', hasProgressImage: false })); - expect(h.isRevealed()).toBe(false); - const h2 = createHarness(); - h2.controller.run(rendering('a.png')); - h2.controller.run( - inputs({ renderedItemName: 'b.png', selectedItemName: 'b.png', shouldShowProgressInViewer: false }) - ); - expect(h2.isRevealed()).toBe(false); + h.select('b.png'); + h.sync({ isProgressImageResolving: true }); + h.syncRendered('b.png', { isProgressImageResolving: true }); + expect(h.isRevealed(), 'the hand-off owns the viewer while it runs').toBe(false); + + h.syncRendered('b.png'); + expect(h.isRevealed(), 'and the click is honoured once it ends').toBe(true); }); - it('keeps a reveal the user already earned when a generation elsewhere starts resolving', () => { - // The clicked item is on screen with its two seconds running. Another session finishing is no - // reason to slam the opaque overlay back over it. + it('keeps a reveal already granted when a resolve window starts under it', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); + h.select('b.png'); + h.syncRendered('b.png'); expect(h.isRevealed()).toBe(true); - h.controller.run({ ...rendering('b.png'), isProgressImageResolving: true }); - expect(h.isRevealed(), 'the granted reveal survives the resolve window').toBe(true); + h.syncRendered('b.png', { isProgressImageResolving: true }); + expect(h.isRevealed()).toBe(true); + }); - // ...and it still ends on its own rather than sticking there. - h.fireTimers(); + it('drops the reveal when the progress overlay goes away', () => { + const h = createHarness(); + h.select('b.png'); + h.syncRendered('b.png'); + h.syncRendered('b.png', { hasProgressImage: false }); expect(h.isRevealed()).toBe(false); + expect(h.pendingTimerCount(), 'and takes its timer with it').toBe(0); }); - it('lowers during a resolve window when the in-flight reveal is for a different item', () => { + it('drops the reveal when the selection is cleared', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); - expect(h.isRevealed()).toBe(true); - h.controller.run({ ...rendering('c.png'), isProgressImageResolving: true }); + h.select('b.png'); + h.syncRendered('b.png'); + h.select(null); + h.sync(); expect(h.isRevealed()).toBe(false); }); - it('leaves no stale timer behind when a run supersedes a reveal', () => { - // Two timers alive at once means the older one lowers the newer one's reveal early. + it('keeps at most one timer alive across a run of selections', () => { + // Two live timers means the older one lowers the newer reveal early. const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run(rendering('b.png')); - h.controller.run(rendering('c.png')); + h.select('a.png'); + h.syncRendered('a.png'); + h.select('b.png'); + h.syncRendered('b.png'); + h.select('c.png'); + h.syncRendered('c.png'); expect(h.pendingTimerCount()).toBe(1); - - h.controller.run({ ...rendering('c.png'), isProgressImageResolving: true }); - expect(h.pendingTimerCount(), 'the resolve-window re-arm replaces the timer, not adds one').toBe(1); }); - it('reveals the same item re-selected after a clear that happened during a resolve window', () => { - // The clear has to be recorded even while resolving: leaving the ref on the item that was - // cleared makes re-selecting it read as "nothing changed" when the window ends, and the - // re-click stays hidden under the overlay. + it('is idempotent across repeated syncs of the same inputs (StrictMode double-invoke)', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - - const resolving = { isProgressImageResolving: true }; - h.controller.run(inputs({ ...resolving, renderedItemName: null, selectedItemName: null })); - h.controller.run({ ...rendering('a.png'), ...resolving }); - - // The resolve window ends with the same item selected again. - h.controller.run(rendering('a.png')); - expect(h.isRevealed()).toBe(true); + h.select('b.png'); + h.syncRendered('b.png'); + const revealedAfterFirst = h.isRevealed(); + h.syncRendered('b.png'); + h.syncRendered('b.png'); + expect(h.isRevealed()).toBe(revealedAfterFirst); + expect(h.pendingTimerCount()).toBe(1); }); - it('reveals the first click made from an empty viewer during a generation', () => { - // The viewer has never shown an item, so there is no previous one to compare against — but the - // user's click is still a click, and suppressing it leaves their pick behind the overlay for - // the rest of the render. + it('holds a claim that has not been shown when a hand-off begins', () => { + // The reveal is owed but not yet visible: dropping it here would lose the click entirely, + // since nothing else remembers it. const h = createHarness(); - h.controller.run(inputs({ renderedItemName: null, selectedItemName: null })); + h.select('b.png'); + h.sync({ renderedItemName: 'b.png', isMediaReady: false }); + h.sync({ renderedItemName: 'b.png', isMediaReady: false, isProgressImageResolving: true }); + expect(h.isRevealed()).toBe(false); + + // The media arrives during the window — still not shown, the hand-off owns the viewer. + h.syncRendered('b.png', { isProgressImageResolving: true }); + expect(h.isRevealed()).toBe(false); - h.controller.run(rendering('first-pick.mp4')); + // ...and it is honoured once the window ends. + h.syncRendered('b.png'); expect(h.isRevealed()).toBe(true); }); - it('still does not reveal the render that happens when the viewer opens on a selection', () => { - // Nothing was clicked; the viewer is just catching up with a selection that already existed. + it('does not reveal a superseded item when its media finally arrives', () => { + // The slow video's frame lands after the user has moved on. Revealing it then would show them + // an item they are no longer looking at, over a live preview. const h = createHarness(); - h.controller.run(inputs({ renderedItemName: null, selectedItemName: 'already.png' })); - h.controller.run(rendering('already.png')); - expect(h.isRevealed()).toBe(false); - }); + h.select('slow.mp4'); + h.sync({ renderedItemName: 'slow.mp4', isMediaReady: false }); - it('does not spend the reveal on a video that has not decoded a frame yet', () => { - // preload="metadata" and the near-zero seek do not prove a frame exists. Starting the two - // seconds at mount let them run out over a black element, then put the overlay back. - const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run({ ...rendering('slow.mp4'), isMediaReady: false }); - expect(h.isRevealed()).toBe(false); + h.select('next.png'); + h.sync({ renderedItemName: 'slow.mp4', isMediaReady: false }); - h.controller.run(rendering('slow.mp4')); - expect(h.isRevealed(), 'the reveal starts when the frame is there').toBe(true); + h.syncRendered('slow.mp4'); + expect(h.isRevealed()).toBe(false); }); - it('reveals media that never becomes ready rather than swallowing the click', () => { + it('settles a selection that lands while no preview is mounted, rather than replaying it', () => { + // Comparison mode keeps the provider (and the progress preview) alive while unmounting both + // preview components, so nothing syncs. Returning must not fire a reveal for a click made + // while no overlay was covering anything. const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run({ ...rendering('broken.mp4'), isMediaReady: false }); + h.select('a.png'); + h.syncRendered('a.png'); h.fireTimers(); - expect(h.isRevealed()).toBe(true); - }); - it('keeps holding a claim across re-runs while the frame is still missing', () => { - // StrictMode re-runs the mount effect; the wait must not restart from zero each time, nor be - // dropped as "nothing changed". - const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run({ ...rendering('slow.mp4'), isMediaReady: false }); - h.controller.run({ ...rendering('slow.mp4'), isMediaReady: false }); - expect(h.isRevealed()).toBe(false); - expect(h.pendingTimerCount()).toBe(1); + const detach = h.machine.attach(); + detach(); + h.select('picked-while-comparing.png'); + h.noteSelection(); - h.controller.run(rendering('slow.mp4')); - expect(h.isRevealed()).toBe(true); + h.syncRendered('picked-while-comparing.png'); + expect(h.isRevealed()).toBe(false); }); - it('carries an unpainted claim through a resolve window that starts under it', () => { - // Mid-generation the user clicks a video that has not decoded yet, and the running item then - // completes, opening its resolve window. The claim must survive the window: dropping it there - // leaves the next run seeing "same item, nothing pending" and the click is swallowed forever. + it('ignores noteSelection while a preview is mounted, leaving the decision to sync', () => { const h = createHarness(); - h.controller.run(rendering('a.png')); - h.controller.run({ ...rendering('v.mp4'), isMediaReady: false }); - expect(h.isRevealed()).toBe(false); - - h.controller.run({ ...rendering('v.mp4'), isMediaReady: false, isProgressImageResolving: true }); - expect(h.isRevealed(), 'held, not shown — the hand-off owns the viewer').toBe(false); - - h.controller.run(rendering('v.mp4')); - expect(h.isRevealed(), 'the click is honoured once the window ends and the frame is there').toBe(true); + const detach = h.machine.attach(); + h.select('b.png'); + h.noteSelection(); + h.syncRendered('b.png'); + expect(h.isRevealed()).toBe(true); + detach(); }); }); diff --git a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts index d385863de29..22b24eae58b 100644 --- a/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts +++ b/invokeai/frontend/web/src/features/gallery/components/ImageViewer/selectedItemReveal.ts @@ -1,85 +1,88 @@ +import type { GallerySelectionDescriptor } from 'features/gallery/store/gallerySelectionSource'; + /** - * The viewer's "reveal selected item" mechanism, extracted from the preview components so its - * sequencing is testable without rendering. + * The viewer's "reveal the selected item" mechanism, as one item-owned state machine. * * A generation covers the viewer with an opaque progress overlay, which would otherwise swallow * every gallery click for the whole render: the selection changes underneath, but nothing visibly - * happens. The reveal lifts the overlay for `durationMs` when the rendered item changes because - * the *user* picked a new one, then drops back to the live preview. - * - * CurrentImagePreview and CurrentVideoPreview each own one controller and call `run` from the - * effect that reacts to their rendered item; `lastRenderedItemNameRef` is shared between them via - * the viewer context so a click that switches media type still reads as a selection change. + * happens. The reveal lifts the overlay for `durationMs` when the *user* picks something, then + * drops back to the live preview. * - * Sequencing rules the controller encodes (each one has a failure mode without it): + * The machine is owned by the viewer context, so both preview components drive one instance and a + * click that switches media type is just another selection. What used to carry that across the + * component swap — a shared mutable ref holding the previously rendered name, a sentinel value for + * "the selection was cleared", and one controller per component — is gone: a selection is identified + * by `generation`, so "the user picked X", "X has been on screen for a while" and "the user picked X + * again" are three distinguishable events rather than three readings of one string comparison. * - * - The previous-item ref is NOT advanced, and the auto-switch marker NOT consumed, while a - * finished render's preview is resolving into its final frame. A click landing inside that - * window keeps its identity until the window ends, so the next run can still classify it — - * revealing a user click that resumes progress would otherwise be impossible, and an - * auto-switch would consume its marker on a run that can never reveal, then read as a user - * click afterwards. - * - The marker IS consumed on every other change of the rendered item, even when no progress is - * showing: it must not outlive the render it was recorded for. - * - Clearing the selection moves the ref to a sentinel rather than null: null means "nothing has - * rendered since the viewer opened" and suppresses the reveal (that first render is not a - * click), but the selection made after a clear IS a click and must reveal — including a - * re-selection of the very item that was cleared. - * - A run that finds the rendered item unchanged while this controller's own reveal is the one - * in flight re-arms it instead of killing it. React StrictMode double-invokes a mount's - * effects (run → cleanup → run), so without this every cross-media first reveal dies in - * development: the second run sees the name the first run wrote into the shared ref. - * - Every other path lowers the reveal. `run` cancels the running reveal's timer before - * deciding, so an outcome that left the flag raised would have nothing left to lower it. + * A reveal is owed from the moment the selection lands, but it is only *shown* once that item's + * media can actually be seen. Lifting the overlay onto an element that has not decoded a frame yet + * shows the user a black rectangle where their click should be — so the machine waits for the item + * to be rendered and for `isMediaReady`. Once the item is rendered, the wait for its media is + * bounded by `mediaGraceMs`, so that media which never becomes ready (a failed load, a codec the + * browser will not decode) still makes the click land rather than swallowing it. Until the item + * renders there is nothing sensible to lift the overlay onto — only the previous item or a blank + * element — so the claim stays outstanding instead of running against a deadline. */ - type SelectedItemRevealInputs = { + /** Who selected what, and which selection it is (see gallerySelectionSource). */ + selection: GallerySelectionDescriptor; + /** The item the component is rendering right now, or null when it has nothing to show. */ + renderedItemName: string | null; /** Whether the rendered item has a frame on screen: an image decoded, a video's first frame - * painted. A