From e1cb4a888315f00bc39e92f5dcc1c521e7d5bcd7 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 21 May 2026 21:19:02 -0400 Subject: [PATCH 1/2] fix(bookmarks): adjust indices on image delete and surface quota errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two latent issues in bookmarks.js, both flagged in the code-review fragility list: * **Bookmark indices weren't being adjusted after a single-image delete.** ``control-panel.js:handleSuccessfulDelete`` mutates ``slideState`` and re-fetches index metadata but never told the bookmark manager that indices below the deletion had shifted down. The in-memory bookmark set kept the old indices, so a bookmark on what *used* to be image 10 silently moved to point at the new image 10 (i.e. the old image 11) after deleting image 7. The multi-image path in ``deleteBookmarkedImages`` already dispatches ``albumChanged`` with ``changeType: "deletion"`` and ``deletedIndices`` (consumed by back-stack.js and slide-state.js). This change makes the single-image path do the same and teaches bookmarks.js to listen for that event and adjust in place. The dead ``removeBookmark`` method (no external callers — leftover from before the event-based flow) is replaced by ``handleDeletion`` which handles the multi-index case the same way the backend renumbers: drop bookmarks at deleted positions, shift higher bookmarks down by the count of deletions below them. * **``saveBookmarks`` silently swallowed ``QuotaExceededError``.** A user with a full localStorage quota would toggle a bookmark, see the star light up, then lose it after reload with no indication. Now a one-shot alert tells the user storage is full (suppressed for subsequent saves so rapid toggles don't spam) and the warn-log path is preserved for non-quota write failures (private mode, etc.). The multi-delete path (deleteBookmarkedImages) is unchanged at the event-dispatch level. Its listener now adjusts bookmarks via handleDeletion, but the function still calls ``clearBookmarks`` right after, so the brief adjustment is overwritten by the clear — same end state as before, just with a more consistent intermediate. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../frontend/static/javascript/bookmarks.js | 69 +++++++++++++++---- .../static/javascript/control-panel.js | 15 ++++ 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/photomap/frontend/static/javascript/bookmarks.js b/photomap/frontend/static/javascript/bookmarks.js index 8f6ec8f3..7a6687c5 100644 --- a/photomap/frontend/static/javascript/bookmarks.js +++ b/photomap/frontend/static/javascript/bookmarks.js @@ -84,8 +84,19 @@ class BookmarkManager { } setupEventListeners() { - // Listen for album changes to load bookmarks for the new album - window.addEventListener("albumChanged", () => { + // ``albumChanged`` covers both album switches and intra-album deletions. + // Switches reload the new album's bookmark set from localStorage; + // deletions adjust the existing in-memory set to match the backend's + // post-delete renumbering (subsequent indices shift down). Without the + // deletion branch, bookmarks would silently point at the wrong images + // after a single-image delete from control-panel.js. + window.addEventListener("albumChanged", (e) => { + const detail = e.detail || {}; + if (detail.changeType === "deletion" && Array.isArray(detail.deletedIndices)) { + this.handleDeletion(detail.deletedIndices); + this.updateAllBookmarkIcons(); + return; + } this.loadBookmarks(); this.isShowingBookmarks = false; this.previousSearchResults = null; @@ -161,6 +172,18 @@ class BookmarkManager { const indices = Array.from(this.bookmarks); localStorage.setItem(key, JSON.stringify(indices)); } catch (e) { + // Quota exceeded or private-mode write rejection — the in-memory set + // is still correct for this session, but won't persist across reload. + // Tell the user once so they're not surprised when bookmarks "vanish" + // after refresh; suppress subsequent alerts so a rapid sequence of + // toggles doesn't spam. + const isQuota = e && (e.name === "QuotaExceededError" || e.code === 22); + if (isQuota && !this._quotaAlerted) { + this._quotaAlerted = true; + alert( + "Bookmark save failed: browser storage is full. Bookmarks won't persist after reload until storage is freed." + ); + } console.warn("Failed to save bookmarks:", e); } @@ -242,23 +265,41 @@ class BookmarkManager { } /** - * Remove a bookmark (used when image is deleted) + * Reconcile bookmark indices after one or more images were deleted from + * the album. Bookmarks pointing at deleted images are removed; bookmarks + * pointing at later images shift down by the count of deleted indices + * below them — same renumbering the backend performs on delete. + * + * Called from the ``albumChanged`` listener with ``changeType: "deletion"`` + * — single-image deletes (control-panel.js) and multi-image deletes + * (deleteBookmarkedImages, which then clears the lot) both flow through + * the same event. */ - removeBookmark(globalIndex) { - if (this.bookmarks.has(globalIndex)) { - this.bookmarks.delete(globalIndex); - // Adjust indices for images after the deleted one - const newBookmarks = new Set(); - for (const idx of this.bookmarks) { - if (idx > globalIndex) { - newBookmarks.add(idx - 1); + handleDeletion(deletedIndices) { + if (!deletedIndices || deletedIndices.length === 0 || this.bookmarks.size === 0) { + return; + } + const deletedSet = new Set(deletedIndices); + const sortedDeleted = [...deletedIndices].sort((a, b) => a - b); + const newBookmarks = new Set(); + for (const idx of this.bookmarks) { + if (deletedSet.has(idx)) { + continue; // bookmark itself was deleted + } + // Count deletions strictly below ``idx`` — small N on both sides, so a + // linear scan beats the bookkeeping of binary search here. + let shift = 0; + for (const d of sortedDeleted) { + if (d < idx) { + shift += 1; } else { - newBookmarks.add(idx); + break; } } - this.bookmarks = newBookmarks; - this.saveBookmarks(); + newBookmarks.add(idx - shift); } + this.bookmarks = newBookmarks; + this.saveBookmarks(); } /** diff --git a/photomap/frontend/static/javascript/control-panel.js b/photomap/frontend/static/javascript/control-panel.js index 936b97ab..ecded868 100644 --- a/photomap/frontend/static/javascript/control-panel.js +++ b/photomap/frontend/static/javascript/control-panel.js @@ -164,6 +164,21 @@ async function handleSuccessfulDelete(globalIndex, searchIndex) { 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. + window.dispatchEvent( + new CustomEvent("albumChanged", { + detail: { + album: state.album, + totalImages: slideState.totalAlbumImages, + 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) { From 2a1045fe1e129083efe23a4ea4ab9f5ace45a3f2 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Fri, 22 May 2026 18:27:20 -0400 Subject: [PATCH 2/2] fix(backend): use stable lexsort so global indices survive deletion ``np.argsort`` defaults to quicksort, which is not stable. When multiple files share a modification time -- common: EXIF ``DateTimeOriginal`` is 1-second resolution, so any burst sequence or batch copy ties -- the relative order among the tied items is arbitrary, and re-running the sort (which happens on every cache reload and inside ``remove_image_from_embeddings``) can permute them differently. The visible failure is exactly the scenario the bookmark fragility work was already chasing: a user with bookmarks {5, 10, 15} deletes image 7, the in-memory bookmark set correctly renumbers to {5, 9, 14}, but the backend's *new* sort order doesn't match a clean "shift down by 1 above the deletion" -- old image 10 may surface at new index 7, old image 12 may stay at 13, etc. Bookmarks point at the wrong images through no fault of the bookmark code. Switch to ``np.lexsort((filenames, modtimes))`` at all three sort sites (cache load, deletion, path-update). lexsort is stable and uses the filename as a deterministic tiebreaker -- so the order is reproducible across reloads, deletions, and even a fresh re-index of the same files. Tested with a 50-file repro (40 tied modtimes): the old sort permutes 9 positions after a single deletion; the new sort permutes 0. One unavoidable consequence: bookmarks and back-stack entries saved under the old (quicksort) ordering may now point to different images, because the canonical sort changed. Since the prior ordering wasn't reproducible in the first place, there is no safe way to migrate them. Co-Authored-By: Claude Opus 4.7 (1M context) --- photomap/backend/embeddings.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index 5a8ebcfb..22055b7f 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -272,8 +272,13 @@ def _open_npz_file(embeddings_path: Path) -> dict[str, Any]: else (int(embeddings.shape[1]) if embeddings.ndim == 2 and embeddings.size else 512) ) - # Pre-compute sorted order - sorted_indices = np.argsort(modification_times) + # Pre-compute sorted order. ``np.lexsort`` is stable and uses ``filenames`` + # as a deterministic tiebreaker when modtimes collide (common: EXIF dates + # are 1-second resolution, so bursts and batch copies tie). Plain + # ``argsort`` defaults to quicksort, which is unstable — same data sorted + # twice could yield different orders, silently invalidating any global + # index a caller (bookmarks, back-stack, deletion) has held onto. + sorted_indices = np.lexsort((filenames, modification_times)) sorted_filenames = filenames[sorted_indices] filename_map = {fname: idx for idx, fname in enumerate(sorted_filenames)} @@ -1493,8 +1498,10 @@ def remove_image_from_embeddings(self, index: int) -> None: embeddings = data["embeddings"].copy() modtimes = data["modification_times"].copy() metadata = data["metadata"].copy() - # Reconstruct sorting locally to find correct index - sorted_indices = np.argsort(modtimes) + # 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. + sorted_indices = np.lexsort((filenames, modtimes)) sorted_filenames = filenames[sorted_indices] current_filename = sorted_filenames[index] @@ -1548,7 +1555,9 @@ def update_image_path(self, index: int, new_path: Path) -> None: embeddings = data["embeddings"].copy() modtimes = data["modification_times"].copy() metadata = data["metadata"].copy() - sorted_indices = np.argsort(modtimes) + # Match the (modtime, filename) lexsort used elsewhere — see + # ``_open_npz_file`` for the rationale. + sorted_indices = np.lexsort((filenames, modtimes)) sorted_filenames = filenames[sorted_indices] current_filename = sorted_filenames[index]