fix: make favorite writes idempotent and ordered - #183
Merged
Conversation
Double-click the star and the UI and the database disagreed permanently: the star read unstarred, the row was in IndexedDB, and a reload brought it back. sdk.favorites.toggle re-derived from the database a decision the caller had already made — a read-modify-write with no ordering guarantee. Two calls in the same tick both read "not favorited", so both wrote, while the in-memory store had toggled twice and settled on "not favorited". Replaced with set(refUri, label, favorite). Taking the desired state as an argument removes the read; a write queue — the same pattern the Trail API already uses for the same reason — keeps two writes for one refUri in the order they were requested, so the last one wins. Re-starring something already starred keeps its original createdAt, so a no-op write cannot move it to the top of the newest-first list on the Browser home. Found by probing, not by reading: firing two clicks in one tick and then comparing the rendered state against IndexedDB directly. The overlap test was checked to fail without the queue.
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
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.
A second bug in #171, found by probing rather than re-reading.
Reproduction
Double-click the star:
The UI and the database disagreed permanently. The star read unstarred, the row was there, and a reload brought it back.
Why
sdk.favorites.toggle(refUri, label)re-derived from the database a decision the caller had already made — a read-modify-write with no ordering guarantee. Two calls in one tick both read "not favorited", so both wrote, while the in-memory store had toggled twice and settled on "not favorited".The fix
set(refUri, label, favorite). Taking the desired state as an argument removes the read entirely, and a write queue — the same pattern the Trail API already uses for exactly this reason — keeps two writes for one refUri in the order they were requested, so the last one wins.Re-starring something already starred keeps its original
createdAt, so a no-op write can't quietly move it to the top of the newest-first list on the Browser home.Verification
Four new SDK tests: set/clear, idempotence (including that
createdAtdoesn't move), clearing something absent, and overlapping writes. The overlap test was checked to fail without the queue —expected [ { …(3) } ] to have a length of +0 but got 1.Note on the API change
toggleis gone rather than kept alongsideset. It only ever had one caller, and leaving a read-modify-write primitive available invites the same bug back in.