From 1100cf747a964691f5d1192f3c8a097b17b7bfe3 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 15:04:19 -0400 Subject: [PATCH 1/2] fix: deleting an image no longer breaks search or drops out of search mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting (or moving) an image rewrote embeddings.npz with only the four per-image arrays, silently stripping the model_id encoder stamp. The next search then misread the index as the legacy openai-clip:ViT-B/32 encoder and failed with EmbeddingCacheMismatch on any album using a different encoder, until a full re-index. Both rewrite paths now carry over every non-per-image key (model_id, embedding_dim, and anything added later). Separately, deleting a search result kicked the user out of search mode and back to slide 1: slide-state's albumChanged handler ran before control-panel's search-preserving code, exited search mode, and reset the position, so the preserving code was dead. The deletion branch in slide-state.js now owns repositioning — it drops deleted entries from the search results in place (state.searchResults shares the array object), renumbers the survivors' global indices, and keeps the search position so the next result fills the slot. This also fixes bookmark multi-delete during an active search, which went through the same path. Co-Authored-By: Claude Fable 5 --- photomap/backend/embeddings.py | 20 +++ .../static/javascript/control-panel.js | 47 ++--- .../frontend/static/javascript/slide-state.js | 62 +++++-- tests/backend/test_index.py | 43 +++++ tests/frontend/slide-state-deletion.test.js | 168 ++++++++++++++++++ 5 files changed, 291 insertions(+), 49 deletions(-) create mode 100644 tests/frontend/slide-state-deletion.test.js 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/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/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); + }); +}); From 0a32e3d02d16bd9dee7fa8df77a3694d96a665e5 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 15:15:04 -0400 Subject: [PATCH 2/2] fix: reword the slow-autotag toast so it doesn't misattribute label recomputes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Preparing autotagging vocabulary — this is usually a one-time operation" toast fires on ANY /cluster_labels or /image_label request still pending after 3s. After a deletion the slow part is the UMAP refit and labels-cache rebuild, not the vocab build (the vocab cache is keyed on the album's encoder spec, which didn't change), so the old wording both blamed the wrong work and broke its "one-time" promise. Use a generic message that covers both causes. Co-Authored-By: Claude Fable 5 --- .../static/javascript/cluster-utils.js | 20 +++++++++++-------- tests/frontend/cluster-utils.test.js | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) 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/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);