fix(bookmarks): adjust indices on image delete and surface quota errors#264
Merged
Conversation
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) <noreply@anthropic.com>
``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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two latent bugs
Both flagged in the code-review fragility list:
1. Bookmark indices weren't 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 dispatched `albumChanged` with `changeType: "deletion"` and `deletedIndices` (consumed by `back-stack.js` and `slide-state.js`). This change:
2. `saveBookmarks` silently swallowed `QuotaExceededError`
A user with a full localStorage quota would toggle a bookmark, see the star light up, and lose it after reload with no indication. Now a one-shot alert tells the user storage is full (suppressed for subsequent saves in the session so rapid toggles don't spam), and the warn-log path is preserved for non-quota write failures (private mode, network volumes, etc.).
Behavior preserved
Test plan
Net +56 lines across 2 files. `bookmarks.js` picks up `handleDeletion` + the deletion-branch listener + the quota alert; `control-panel.js` gets a 15-line event dispatch.
🤖 Generated with Claude Code