diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index 2124d3e0..beff42ab 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -335,6 +335,22 @@ def peek_encoder_spec(embeddings_path: Path) -> str: return LEGACY_ENCODER_SPEC +# Per-image arrays mutated by the delete/move rewrite paths. Every OTHER key +# in the .npz (model_id, embedding_dim, and anything added later) must be +# carried over verbatim when rewriting the file. +_PER_IMAGE_KEYS = frozenset({"filenames", "embeddings", "modification_times", "metadata"}) + + +def _copy_non_per_image_keys(data: Any) -> dict[str, Any]: + """Copy the non-per-image keys out of an open .npz for a rewrite. + + Dropping these on a rewrite silently strips the encoder stamp: the next + reader falls back to ``LEGACY_ENCODER_SPEC`` and every search fails with + :class:`EmbeddingCacheMismatch` until a full re-index. + """ + return {key: data[key].copy() for key in data.files if key not in _PER_IMAGE_KEYS} + + @functools.lru_cache(maxsize=3) def _open_npz_file(embeddings_path: Path) -> dict[str, Any]: """ @@ -1918,6 +1934,7 @@ def remove_image_from_embeddings(self, index: int) -> None: embeddings = data["embeddings"].copy() modtimes = data["modification_times"].copy() metadata = data["metadata"].copy() + extras = _copy_non_per_image_keys(data) # Reconstruct sorting locally to find correct index. Must match # the (modtime, filename) lexsort used in ``_open_npz_file`` or # we'd find the wrong file to delete. @@ -1948,6 +1965,7 @@ def remove_image_from_embeddings(self, index: int) -> None: filenames=filenames, modification_times=modtimes, metadata=metadata, + **extras, ) # 6. Re-prime the cache immediately to verify the write @@ -1975,6 +1993,7 @@ def update_image_path(self, index: int, new_path: Path) -> None: embeddings = data["embeddings"].copy() modtimes = data["modification_times"].copy() metadata = data["metadata"].copy() + extras = _copy_non_per_image_keys(data) # Match the (modtime, filename) lexsort used elsewhere — see # ``_open_npz_file`` for the rationale. sorted_indices = np.lexsort((filenames, modtimes)) @@ -2014,6 +2033,7 @@ def update_image_path(self, index: int, new_path: Path) -> None: filenames=filenames, modification_times=modtimes, metadata=metadata, + **extras, ) logger.info(f"Updated path in embeddings: {current_filename} -> {new_path}") diff --git a/photomap/frontend/static/javascript/cluster-utils.js b/photomap/frontend/static/javascript/cluster-utils.js index 1364d77d..24caeeeb 100644 --- a/photomap/frontend/static/javascript/cluster-utils.js +++ b/photomap/frontend/static/javascript/cluster-utils.js @@ -96,20 +96,24 @@ export function clearImageLabelCache() { // Slow-vocab-build toast // --------------------------------------------------------------------------- // -// `/cluster_labels` and `/image_label` both trigger the server-side vocab -// embedding build the first time they're hit after startup or after the -// album's encoder changes. The build encodes a few thousand phrases through -// CLIP/SigLIP and can take 20-30s on CPU. Without feedback the UI just looks -// frozen. We track in-flight vocab-triggering requests with a counter and -// show a single sticky toast if any of them is still pending after a short -// grace period; the toast is dismissed as soon as the count returns to zero. +// `/cluster_labels` and `/image_label` can both be slow for two different +// server-side reasons: the vocab embedding build (first hit after startup or +// after the album's encoder changes — a few thousand phrases through +// CLIP/SigLIP, 20-30s on CPU) and the label recompute after the index +// changes (deleting an image invalidates umap.npz and the labels cache, so +// the next request refits UMAP over the whole album). The toast can't tell +// which is happening — it fires on any tracked request still pending after +// the grace period — so the message must stay generic; don't reword it to +// promise a "one-time" operation. We track in-flight requests with a counter +// and show a single sticky toast, dismissed as soon as the count returns to +// zero. // // Threshold is generous (3s) so a warm-cache call (sub-second) never flashes // a toast. Exposed via `_setSlowVocabDelayMsForTests` so the Jest test can // shorten it without depending on real timers. const DEFAULT_SLOW_VOCAB_DELAY_MS = 3000; -const SLOW_VOCAB_MESSAGE = "Preparing autotagging vocabulary — this is usually a one-time operation."; +const SLOW_VOCAB_MESSAGE = "Computing autotag labels — this can take a while on large albums."; let slowVocabDelayMs = DEFAULT_SLOW_VOCAB_DELAY_MS; let slowVocabInFlight = 0; diff --git a/photomap/frontend/static/javascript/control-panel.js b/photomap/frontend/static/javascript/control-panel.js index 7140d66d..f8e033e8 100644 --- a/photomap/frontend/static/javascript/control-panel.js +++ b/photomap/frontend/static/javascript/control-panel.js @@ -92,7 +92,7 @@ function handleCopyText() { // Delete the current file async function handleDeleteCurrentFile() { - const [globalIndex, , searchIndex] = getCurrentSlideIndex(); + const [globalIndex] = getCurrentSlideIndex(); const currentFilepath = await getCurrentFilepath(); if (globalIndex === -1 || !currentFilepath) { @@ -108,7 +108,7 @@ async function handleDeleteCurrentFile() { try { showSpinner(); await deleteImage(state.album, globalIndex, state.moveToTrash); - await handleSuccessfulDelete(globalIndex, searchIndex); + await handleSuccessfulDelete(globalIndex); hideSpinner(); } catch (error) { hideSpinner(); @@ -160,49 +160,28 @@ async function confirmDelete(filepath, globalIndex) { return await showDeleteConfirmModal(filepath, globalIndex); } -async function handleSuccessfulDelete(globalIndex, searchIndex) { +async function handleSuccessfulDelete(globalIndex) { const metadata = await getIndexMetadata(state.album); - slideState.totalAlbumImages = metadata?.filename_count || 0; - - // Tell the rest of the app (bookmarks, back-stack, grid view) which index - // the backend just renumbered out from under them. The multi-delete path in - // bookmarks.js fires the same event with multiple indices, so listeners - // only need to handle one shape. + const totalImages = metadata?.filename_count || 0; + + // Tell the rest of the app (slide state, bookmarks, back-stack, grid view) + // which index the backend just renumbered out from under them. The + // multi-delete path in bookmarks.js fires the same event with multiple + // indices, so listeners only need to handle one shape. slide-state.js owns + // repositioning — including staying inside an active search — so don't + // touch slideState's position fields here. window.dispatchEvent( new CustomEvent("albumChanged", { detail: { album: state.album, - totalImages: slideState.totalAlbumImages, + totalImages, changeType: "deletion", deletedIndices: [globalIndex], }, }) ); - // Drop the deleted entry from search results and decrement subsequent global indices. - // Stay on the same search position so the next result fills the slot; clamp at the end. - if (slideState.isSearchMode && slideState.searchResults?.length > 0) { - slideState.searchResults.splice(searchIndex, 1); - for (const result of slideState.searchResults) { - if (result.index > globalIndex) { - result.index -= 1; - } - } - if (slideState.searchResults.length === 0) { - slideState.exitSearchMode(); - } else { - slideState.currentSearchIndex = Math.min(slideState.currentSearchIndex, slideState.searchResults.length - 1); - slideState.currentGlobalIndex = slideState.searchResults[slideState.currentSearchIndex].index; - } - } - - // Backend re-indexes after deletion: the slide that was at globalIndex+1 is now at globalIndex, - // so keeping currentGlobalIndex unchanged naturally advances to the next image. Clamp so that - // deleting the last image lands on the new last image. - if (slideState.totalAlbumImages > 0) { - slideState.currentGlobalIndex = Math.min(slideState.currentGlobalIndex, slideState.totalAlbumImages - 1); - } else { - slideState.currentGlobalIndex = 0; + if (totalImages === 0) { state.swiper.removeAllSlides(); return; } diff --git a/photomap/frontend/static/javascript/slide-state.js b/photomap/frontend/static/javascript/slide-state.js index 3a5cc613..0f843d5b 100644 --- a/photomap/frontend/static/javascript/slide-state.js +++ b/photomap/frontend/static/javascript/slide-state.js @@ -267,27 +267,59 @@ class SlideStateManager { return; } - // For deletions, try to preserve position by calculating how many images before current were deleted - if (detail.changeType === "deletion" && detail.deletedIndices && !this.isSearchMode) { + // For deletions, preserve the current position — and any active search — + // instead of jumping back to the first slide of the album. + if (detail.changeType === "deletion" && detail.deletedIndices) { const deletedIndices = detail.deletedIndices; - const currentIndex = this.currentGlobalIndex; - - // Count how many deleted images were before the current position - const deletedBefore = deletedIndices.filter((idx) => idx < currentIndex).length; + this.totalAlbumImages = detail.totalImages; - // Adjust current position by subtracting deleted images before it - const newIndex = Math.max(0, currentIndex - deletedBefore); + if (this.isSearchMode && this.searchResults.length > 0) { + // Drop the deleted entries from the search results and renumber the + // survivors' global indices (the backend renumbers the album after a + // deletion). Mutate the array in place: ``state.searchResults`` + // (search.js) shares this same array object, and dispatching + // ``searchResultsChanged`` instead would re-enter search mode back + // at position 0. + const deletedSet = new Set(deletedIndices); + const removedBefore = this.searchResults + .slice(0, this.currentSearchIndex) + .filter((r) => deletedSet.has(r.index)).length; + for (let i = this.searchResults.length - 1; i >= 0; i--) { + if (deletedSet.has(this.searchResults[i].index)) { + this.searchResults.splice(i, 1); + } + } + for (const result of this.searchResults) { + result.index -= deletedIndices.filter((idx) => idx < result.index).length; + } + if (this.searchResults.length > 0) { + // Deleting the current result keeps the same search position so the + // next result fills the slot; clamp when the last result was deleted. + this.currentSearchIndex = Math.max( + 0, + Math.min(this.currentSearchIndex - removedBefore, this.searchResults.length - 1) + ); + this.currentGlobalIndex = this.searchResults[this.currentSearchIndex].index; + this.notifySlideChanged(); + return; + } + // Every remaining search result was deleted; fall back to album mode. + this.exitSearchMode(); + } - // Clamp to new total, ensuring non-negative - // Handle edge case where all images are deleted (totalImages = 0) - this.currentGlobalIndex = detail.totalImages > 0 ? Math.max(0, Math.min(newIndex, detail.totalImages - 1)) : 0; - this.currentSearchIndex = 0; - } else { - // For other changes (album switch, move, etc.), reset to beginning - this.currentGlobalIndex = 0; + // Count how many deleted images were before the current position and + // shift back by that many; clamp to the new total (which may be 0). + const deletedBefore = deletedIndices.filter((idx) => idx < this.currentGlobalIndex).length; + const newIndex = Math.max(0, this.currentGlobalIndex - deletedBefore); + this.currentGlobalIndex = detail.totalImages > 0 ? Math.min(newIndex, detail.totalImages - 1) : 0; this.currentSearchIndex = 0; + this.notifySlideChanged(); + return; } + // For other changes (album switch, move, etc.), reset to beginning + this.currentGlobalIndex = 0; + this.currentSearchIndex = 0; this.exitSearchMode(); this.totalAlbumImages = detail.totalImages; // Update from state } diff --git a/tests/backend/test_index.py b/tests/backend/test_index.py index cc70daea..17a083f1 100644 --- a/tests/backend/test_index.py +++ b/tests/backend/test_index.py @@ -114,6 +114,11 @@ def test_delete_image( with np.load(index_path, allow_pickle=True) as data: disk_count = len(data["filenames"]) print(f"Direct disk read count: {disk_count}") + # The rewrite must keep the encoder stamp. Losing it makes readers + # fall back to LEGACY_ENCODER_SPEC and every subsequent search fail + # with EmbeddingCacheMismatch for albums on a non-legacy encoder. + assert "model_id" in data.files, "deletion rewrite dropped model_id" + assert str(data["model_id"]) == new_album["encoder_spec"] assert ( metadata["filename_count"] == TEST_IMAGE_COUNT - 1 @@ -125,6 +130,44 @@ def test_delete_image( ).exists(), "Image file should be deleted" +def test_npz_rewrites_preserve_non_per_image_keys(tmp_path: Path): + """``remove_image_from_embeddings`` and ``update_image_path`` rewrite the + whole ``.npz``; they must carry over ``model_id``, ``embedding_dim``, and + any key added in the future, not just the four per-image arrays. Dropping + ``model_id`` made the next reader fall back to the legacy encoder spec, so + every search on a non-legacy album failed with ``EmbeddingCacheMismatch`` + after a single deletion.""" + encoder_spec = "open-clip:ViT-L-14/dfn2b_s39b" + npz_path = tmp_path / "embeddings.npz" + np.savez( + npz_path, + embeddings=np.eye(3, dtype=np.float32), + filenames=np.array([str(tmp_path / f"{name}.jpg") for name in "abc"]), + modification_times=np.array([1.0, 2.0, 3.0]), + metadata=np.array([{}, {}, {}], dtype=object), + model_id=np.array(encoder_spec), + embedding_dim=np.array(3), + future_key=np.array("still here"), + ) + emb = Embeddings(embeddings_path=npz_path, encoder_spec=encoder_spec) + + emb.remove_image_from_embeddings(0) + with np.load(npz_path, allow_pickle=True) as data: + assert len(data["filenames"]) == 2 + assert str(data["model_id"]) == encoder_spec + assert int(data["embedding_dim"]) == 3 + assert str(data["future_key"]) == "still here" + + emb.update_image_path(0, tmp_path / "renamed.jpg") + with np.load(npz_path, allow_pickle=True) as data: + assert str(tmp_path / "renamed.jpg") in data["filenames"] + assert str(data["model_id"]) == encoder_spec + assert int(data["embedding_dim"]) == 3 + assert str(data["future_key"]) == "still here" + + _open_npz_file.cache_clear() + + # test that we can move images def test_move_images( client: TestClient, new_album: dict, monkeypatch: pytest.MonkeyPatch, tmp_path: Path diff --git a/tests/frontend/cluster-utils.test.js b/tests/frontend/cluster-utils.test.js index 25ad33bb..b6fd7055 100644 --- a/tests/frontend/cluster-utils.test.js +++ b/tests/frontend/cluster-utils.test.js @@ -165,7 +165,7 @@ describe("cluster-utils.js", () => { jest.advanceTimersByTime(50); const toast = document.querySelector(".app-toast"); expect(toast).not.toBeNull(); - expect(toast.textContent).toContain("autotagging vocabulary"); + expect(toast.textContent).toContain("Computing autotag labels"); // Sticky: duration:0 means the auto-dismiss timer never fires. jest.advanceTimersByTime(60_000); expect(document.querySelectorAll(".app-toast")).toHaveLength(1); diff --git a/tests/frontend/slide-state-deletion.test.js b/tests/frontend/slide-state-deletion.test.js new file mode 100644 index 00000000..fea26227 --- /dev/null +++ b/tests/frontend/slide-state-deletion.test.js @@ -0,0 +1,168 @@ +/** + * Tests for slideState's handling of albumChanged with changeType "deletion" + * (dispatched by the trash-can button in control-panel.js and the bookmark + * multi-delete in bookmarks.js): an active search must survive the deletion — + * the deleted entries drop out of the results, the survivors' global indices + * are renumbered, and the position stays put instead of resetting to slide 1. + */ +import { beforeEach, describe, expect, jest, test } from "@jest/globals"; + +const M = "../../photomap/frontend/static/javascript"; + +jest.unstable_mockModule(`${M}/state.js`, () => ({ + state: { album: "alb" }, +})); + +const { slideState } = await import(`${M}/slide-state.js`); + +function dispatchDeletion(deletedIndices, totalImages) { + window.dispatchEvent( + new CustomEvent("albumChanged", { + detail: { album: "alb", totalImages, changeType: "deletion", deletedIndices }, + }) + ); +} + +beforeEach(() => { + slideState.exitSearchMode(); + slideState.currentGlobalIndex = 0; + slideState.currentSearchIndex = 0; + slideState.totalAlbumImages = 0; +}); + +describe("albumChanged changeType deletion — album mode", () => { + test("keeps the position when the deleted image was after it", () => { + slideState.totalAlbumImages = 100; + slideState.currentGlobalIndex = 10; + + dispatchDeletion([50], 99); + + expect(slideState.currentGlobalIndex).toBe(10); + expect(slideState.totalAlbumImages).toBe(99); + expect(slideState.isSearchMode).toBe(false); + }); + + test("shifts the position back past deleted predecessors and clamps at the end", () => { + slideState.totalAlbumImages = 100; + slideState.currentGlobalIndex = 50; + + dispatchDeletion([3, 7], 98); + expect(slideState.currentGlobalIndex).toBe(48); + + slideState.currentGlobalIndex = 97; + dispatchDeletion([97], 97); + expect(slideState.currentGlobalIndex).toBe(96); + }); + + test("goes to 0 when the album empties out", () => { + slideState.totalAlbumImages = 1; + slideState.currentGlobalIndex = 0; + + dispatchDeletion([0], 0); + + expect(slideState.currentGlobalIndex).toBe(0); + expect(slideState.totalAlbumImages).toBe(0); + }); +}); + +describe("albumChanged changeType deletion — search mode", () => { + test("stays in search mode when the current result is deleted (the regression)", () => { + slideState.totalAlbumImages = 100; + const results = [ + { index: 3, score: 0.9 }, + { index: 40, score: 0.8 }, + { index: 77, score: 0.7 }, + ]; + slideState.enterSearchMode(results, 1); + + // Delete the current result (global index 40). + dispatchDeletion([40], 99); + + expect(slideState.isSearchMode).toBe(true); + // Survivors renumbered: 77 shifts down to 76; 3 is untouched. + expect(slideState.searchResults.map((r) => r.index)).toEqual([3, 76]); + // Same search position, so the next result fills the slot. + expect(slideState.currentSearchIndex).toBe(1); + expect(slideState.currentGlobalIndex).toBe(76); + // Must mutate the caller's array in place — state.searchResults + // (search.js) holds the same object and would otherwise go stale. + expect(slideState.searchResults).toBe(results); + expect(results.map((r) => r.index)).toEqual([3, 76]); + }); + + test("clamps to the last result when the final result is deleted", () => { + slideState.totalAlbumImages = 100; + slideState.enterSearchMode( + [ + { index: 3, score: 0.9 }, + { index: 77, score: 0.7 }, + ], + 1 + ); + + dispatchDeletion([77], 99); + + expect(slideState.isSearchMode).toBe(true); + expect(slideState.currentSearchIndex).toBe(0); + expect(slideState.currentGlobalIndex).toBe(3); + }); + + test("keeps pointing at the same result when an earlier result is deleted", () => { + slideState.totalAlbumImages = 100; + slideState.enterSearchMode( + [ + { index: 3, score: 0.9 }, + { index: 40, score: 0.8 }, + { index: 77, score: 0.7 }, + ], + 2 + ); + + dispatchDeletion([3], 99); + + expect(slideState.currentSearchIndex).toBe(1); + expect(slideState.currentGlobalIndex).toBe(76); + }); + + test("multi-delete (bookmarks) removes several results and renumbers the rest", () => { + slideState.totalAlbumImages = 100; + slideState.enterSearchMode( + [ + { index: 3, score: 0.9 }, + { index: 40, score: 0.8 }, + { index: 60, score: 0.75 }, + { index: 77, score: 0.7 }, + ], + 2 + ); + + // Bookmarks deletes globals 3 and 77 — one before the current result, + // one after; global 10 was never in the search results. + dispatchDeletion([3, 10, 77], 97); + + expect(slideState.isSearchMode).toBe(true); + expect(slideState.searchResults.map((r) => r.index)).toEqual([38, 58]); + expect(slideState.currentSearchIndex).toBe(1); + expect(slideState.currentGlobalIndex).toBe(58); + }); + + test("exits search mode only when every result is deleted", () => { + slideState.totalAlbumImages = 100; + slideState.currentGlobalIndex = 40; + slideState.enterSearchMode( + [ + { index: 3, score: 0.9 }, + { index: 40, score: 0.8 }, + ], + 1 + ); + + dispatchDeletion([3, 40], 98); + + expect(slideState.isSearchMode).toBe(false); + expect(slideState.searchResults).toEqual([]); + // Falls back to album mode at the renumbered global position. + expect(slideState.currentGlobalIndex).toBe(39); + expect(slideState.totalAlbumImages).toBe(98); + }); +});