fix(app): serialise quote writes against an incoming sync - #321
Open
kYem wants to merge 31 commits into
Open
Conversation
Every quote writer hoisted its read above the write, so a pull landing in the service worker between the two was erased by whatever the page wrote back. Route them all through updateQuotes, which reads inside the same 'quotes' web lock arrayBinding already takes, and collect the sync notify ids from the locked read so a quote the pull removed is not marked dirty and tombstoned for every other device.
deleteQuote decided its tombstone from the pre-lock snapshot, the rule bulkDelete states one function away, so a quote a pull had already removed was announced as this device's delete and outranked every peer still holding it. The single-quote writers also toasted success for a write that matched nothing, losing the edit silently. Also drop the "seed quotes are excluded" claim: the enroll backfill claims every stored id, seed ones included. Only the announcement is custom-only, which syncsLocalEdits now says once instead of ten times.
Mutation testing put real read-inside-the-lock coverage at 2 of 17 writers: the rest passed whether they read storage or their own snapshot, because the default getQuotes mock mirrors the store. Nineteen tests now fail when the read is hoisted, and eight when the write-failure guard goes. Also stop the sync sink block borrowing setQuotes from the describe above it. Under isolation twelve of its tests failed, and its five negative assertions passed only because the writer threw before reaching a notify.
Removing the stale-snapshot write took away what had been keeping the displayed quote and the stored list in agreement: nothing converges currentQuote, so a write that found the quote gone left the card rendering it across reloads. bulkToggleFavorite was still stamping that snapshot into storage outright. Bulk toasts now count what the locked read matched rather than what was selected, editQuote reports 'gone'/'failed' so the modal can keep a retryable edit on screen, and a pull that beats the seed skips the write instead of failing the load with it.
Nine of eighteen writers still passed with their lock removed entirely, and eight of the sixteen write-failure guards were untested. The worst were the ones with no coverage at all: incrementViewCount runs on every new tab and rewrites the whole list, and addCustomQuote appends, so a stale read there drops every quote a concurrent pull added. Two tables were also passing loosely. The failed-write rows matched a message substring, so editQuote's assertion was satisfied by bulkToggleHidden's copy, and they compared ids where every mutation but a delete leaves the id alone. The announce rows inspected one sink channel, so an id leaking through another was invisible.
A pull that removed everything the user had selected left them reading "Deleted 0 quotes" as a success. The single-quote writers already warn in that case, and restoreMissingQuotes already downgrades its own zero to an info toast.
The card outlived the quote on every bulk path: reading inside the lock turned the currentQuote rewrite into a lookup that can miss, and nothing took the else branch. Bulk unhide never touched currentQuote at all. addQuotesToCollection counted only what it added, so a pull that deleted the whole selection was reported as membership the quotes already had. Clearing the card also stopped at memory: nothing clears the stored key and initialize only rejected a hidden quote, so the next tab restored one the list no longer holds.
The shared table matched one `if (!result.success) { throw }` form, so the
seed write, restoreMissingQuotes, resetAllQuotes and bulkAddQuotes were
never pinned — each could lose its guard with the suite green.
Also correct the fresh-array note in the converging block: the no-op guard
compares by value; it is the test below that asserts reference identity,
and one shared array satisfies that either way.
clearCurrentQuoteIfGone never checked whether the quote was gone — it matched the id and refreshed. That was fine where the caller already knew, but bulk unhide called it for a quote that had survived, so unhiding a batch containing the displayed quote swapped the card, bumped a view count and pushed history for a write that touched only the hidden flag. It now checks against the written list, so the name is true wherever it is called, and addQuotesToCollection gets the guard the other bulk writers already had.
QuoteDisplay and QuoteManagementPage both read `error` as "initialize failed" and render a panel titled "Failed to load quotes" in place of everything. On main a failed write never reached those catch blocks because the result was discarded, so checking it made the collapse reachable: clicking the heart while over quota took out the card, the Space shortcut and the whole quotes page, and the user's route to freeing space was behind the same panel. The writers keep their log and toast. `error` now means only what the two renderers already assume it means.
The freshness tables asserted toContain('late'), so a writer that picked
up the locked read and dropped the quote it was editing satisfied them —
real data loss for every row but the delete. They compare the whole list
now, which fifteen tests catch.
The seed test read setQuotes.mock.calls[0], which is the view-count bump,
not a seed write; bulkToggleFavorite's announce check inspected one sink
channel where the table it belongs in checks all three. Also drop the
QuoteForm reference, a component that does not exist, and move the sink
cleanup into afterEach so a throwing act cannot leak it forward.
Dropping `error` state from the writers removed the panel that had been covering for three callers that never read an outcome. AddQuoteForm was the worst of them: addCustomQuote resolved the same either way, so a failed save wiped every field and closed the form, which is the bug EditQuoteModal was fixed for one file over. addCustomQuote and the three bulk writers now report whether the write landed; the form keeps what was typed, the collection modal stays open on a partial apply, and a failed bulk keeps the selection so the retry does not start from scratch. refreshQuote also stops latching `error` — the new-tab interval calls it unprompted, so a failed write there replaced the card with a load-failure panel the user never triggered.
Neither announced what it changed, so a peer still holding those quotes pushed them straight back: the reset wiped custom quotes locally only, and deleting a collection left its members re-linked on every other device. The reset still replaces the list wholesale — it reads only to learn which ids it destroys. Quota failures also stop advising a retry that cannot work. The writers throw through assertPersisted now, so the StorageError survives as `cause` and the catch can say the disk is full instead. A write aimed at an id the locked read no longer holds is skipped rather than rewritten identically, which stops a pointless failure masking the real answer.
Collecting tombstone ids gave resetAllQuotes a read it never had, and an unreadable quote key throws — so the one lever for recovering a wedged store refused to run, permanently, on exactly the storage it is for. The read is guarded now and raw, which also matches the raw write: the validating read hides the rows that write destroys, so their ids were never getting tombstones. The collection modal could also never close once a pull deleted a selected quote: 'gone' and 'failed' both came back as false, and reapplying could only fail the same way. Those two writers report the outcome the rest already do. Seeding was the one write still dropping its StorageError, and it is the largest one in the store, so a full disk on first run advised a refresh that could not work. assertPersisted also stops reading a missing result as a successful write.
Switching the tombstone read to raw dropped the schema guarantee with it, so a row with no usable id could reach notifyDeleted. The push then fails validation for the whole batch and never clears — every later push from that device fails on it too, until a re-enroll. An unreadable read also left the reset claiming success while authoring no tombstones at all, so the quotes came back from every peer with nothing on screen connecting the two. It warns now, like deleteCollection does. setCurrentQuote was the last storage write in this store whose result was dropped — ten call sites, no log, so the displayed quote could revert on the next tab with no record anywhere.
persistCurrentQuote logged and moved on, so editQuote toasted "Quote updated successfully" while the write that keeps the card in step had failed. initialize reads that key back verbatim, so the card then showed the pre-edit quote on every later tab while the list showed the new one. The six writers whose list write has already landed now say the card is behind instead of claiming success. The helper also dereferenced a result it had just admitted might be nullish — the same shape assertPersisted was hardened against in the commit that introduced it — and let getStorageArea's rejection escape, which reported the whole action as failed for a write that persisted. It answers now, so both exits land in the same place. The reset warning no longer tells a sync-off user their quotes may return from devices they do not have, the modal drops the changes that landed rather than reporting the whole batch as pending, and addQuotesToCollection clears the selection its two siblings already clear.
…s it The message had both nouns backwards. Every emit site sets currentQuote straight after the failed write, so this tab is the one surface showing the new value — it is the stored card other tabs read that is stale. And initialize reads that key back verbatim, so refreshing is what replaces the edit with the old copy, on every reload. The advice reproduced the bug it warned about. refreshQuote no longer warns at all: hideQuote, deleteQuote and bulkDelete all delegate their card write to it, and there the stale key self-heals because initialize discards a card whose quote is gone — so the warning was a false alarm arriving next to a contradicting success toast, and "Saved," was untrue for the unattended interval. The bulk writers also stopped returning early, which had swallowed the matched count an earlier round added to surface a partly-deleted selection. resetAllQuotes is the seventh writer of this shape and now reports it too, and rows skipped for having no id mark the reset as incomplete rather than logging below the shipped log level.
initialize took the stored card's contents on trust and checked only that its id still existed. So a quote another device hid or edited kept rendering on this one — no failure involved, just a snapshot nothing converges. It now resolves that id against the list it just read. That removes the reason the card warnings existed. For every writer that keeps the quote's id, a lost card write is now invisible: the next mount reads the live copy. The message they emitted is gone, and with it the flag six writers threaded through to reach it. The same unvalidated-id hazard the reset guards against was still open in the sync binding: readAll keyed an id-less row as "undefined", which the enroll backfill claims and the server rejects, failing that device's whole push batch on every cycle until a re-enroll.
deleteCollection has reported its outcome since the collections work and nothing read it, so a failed delete closed the dialog and left the collection in the list with an error toast as the only trace. The bulk handlers had the same gap in their tests rather than their code: nothing reached handleBulkUnhide or either favourite handler, so flipping one argument would have inverted the action across the user's whole selection with the suite still green. AddQuoteForm had no test at all, though its early return is the reason addCustomQuote reports anything. The bulk toolbar gets a role and a name so those tests can tell its buttons from the identically titled ones on each quote card.
persistOneQuote's gone path returned `{success: true}` for a write that
never ran, so assertPersisted — the one check keeping an unpersisted list
off the screen — was handed a value nothing vouched for. Only the
correlated `target` could tell the two apart, and the type did not
correlate them: seven callers hand-ordered assert, set, null-check, and
skipping the last was silent.
It answers a discriminated union now. Reading `target` without
discriminating is a compile error, so the ordering is the compiler's to
enforce rather than each caller's to remember, and the fabricated result
has nowhere to live.
The id is also kept from the map instead of re-found afterwards: a change
that rewrote it read as gone after the write had already landed.
Deleting the card warnings was right for the six writers that keep the quote's id — the next mount re-resolves it from the list. It was wrong for the two that change the id: refreshQuote and navigateHistory replace a stored id that is still live, so nothing re-resolves it and the tab the user just navigated reverts everywhere else, silently. refreshQuote now reports only a deliberate reroll, so the interval stays quiet. incrementViewCount was the one gone path that neither reported nor moved the card, which is the phantom an earlier commit removed reaching back through the writer that fix skipped. persistOneQuote also folds the assert in and drops `result`, so both members of the union carry a list storage has vouched for; and `target` is boxed, because a bare `let` narrows to `never` past the guard and the compiler stops checking the guarantee the union exists for.
Giving the view-count gone path a card refresh closed a phantom but tied two writers into a loop: refreshQuote bumps the count, a gone count rerolls the card, and the reroll bumps again. Each hop usually resyncs the list and settles, so it looked safe — but a read that keeps answering a list without the quote just picked never settles. The test that proves it takes the worker down without the guard. A first attempt at that test passed against a deliberately broken version, because an empty list terminates on its own; it needed a read that answers a different quote every time to reach the real path.
Gating the warning on user intent missed a caller: ConceptRotation's skip is a keypress, paired with onManualRefresh for the same reason the space shortcut is, and it passed no options — so the one path the gate exists for stayed silent. The comment justifying the gate enumerated two kinds of caller and this was a third. The refuted sync claim also survived in the test file: the source copy was corrected last round and its duplicate was not, which is the cost of saying the same thing twice made concrete. Inserting the report-once set between arrayBinding's docblock and arrayBinding orphaned it, the same way an earlier round already found once.
Bounding the cycle with a re-entry flag stopped the hang and reopened the phantom it existed to close: the nested call is the card repair, so refusing it left the card on a quote storage had just proven gone. Two reviewers reproduced that with a failing assertion, and one measured the unguarded version dying of heap exhaustion after 31,342 levels. The gone path now picks a replacement from the list already in hand. No call back into refreshQuote, so nothing can recurse, and the card keeps a real quote rather than blanking into "No quotes available". The reset also stopped shadowing its own warning — a card message must not swallow the one saying the wiped quotes may return from other devices — and the gone reports move to error, which is the level shipped builds keep.
The gone path picked with getRandomQuote(list) and no arguments, so the card could land on a category the user turned off, a non-favourite under favourites-only, or a quote outside the collection they were looking at — while refreshQuote passes all four. Same list, same moment, different rules depending on which path repaired the card. The test that would have caught it could not: EMPTY_STORE_STATE never reset the filter fields, so a test that narrowed them left every later test picking from a filtered list. It resets them now, which is what made the spin guard fail the moment a filter was introduced above it.
isCustomQuote counts a favourited or hidden seed quote as custom, so the write moves it between keys. The seed key is written first and drops it, then a failed custom write never takes it — leaving the quote in neither, while the user is told only that the update failed. The observer then makes it disappear from the list with no further word. The seed key is now captured before the write and restored when the second one fails, on the throwing path as well as the resolving one. Also here: initialize and resetAllQuotes were the two remaining unfiltered card picks, the reset dialog closed on a failed reset, and toggling a pending change back left it counted and rewrote the list for nothing.
readAll went straight to storage while writeOne took the lock and said why. Quotes live in two keys, so a favourite moving between them is briefly in neither; a push cycle reading in that window sees no entity and seals a tombstone every other device applies. Unlocked, the new test observes [] where the quote should be — the absence that becomes a delete.
setQuotesRaw never got the try/catch setQuotes has, so getStorageArea rejecting after the seed write skipped the rollback entirely — an earlier commit message claimed parity the code did not have. Both now share one helper rather than two copies that drift. Two reporting bugs in that path: a failed custom write logged nothing at all when the seed snapshot was unreadable, which is the case that loses the quote outright; and `stranded` filtered the pre-write rows by isCustomQuote, so it was empty for exactly the quote at risk. The rollback table covers both writers against a result failure and a throw, and asserts the restored quote is the pre-write copy — an id-only assertion cannot see a rollback that lands the incoming favourite.
resetAllQuotes only moved the card when the pick returned a quote. That else-branch was dead while the pick was unfiltered; filtering it made it live, so a user whose filters exclude every seed quote keeps looking at a custom quote the reset just deleted. deleteQuote asserted the write before checking whether the id was still there, so a delete a pull had already applied surfaced as "storage is full" — telling the user to free space for something already done. incrementViewCount's gone path no longer warns that the card was not remembered: the stored id is the deleted one, so the next mount rerolls and there is nothing for the user to lose. pickQuote names the four filter fields at all four call sites; getRandomQuote takes two adjacent booleans that transpose without a type error.
The setQuotes docblock still said a failed second write leaves the first standing, which the rollback commit made false. Two test comments named scenarios the code cannot produce: navigateHistory skips deleted quotes rather than landing on them, and the claim that every existing case seeded a gone id is refuted by the live-id case at :285. handleBulkDelete's catch closed the dialog, the inverse of the rule stated three lines above it. expectNavigationMethodCalled asserted nothing when the mock store lacked the key, silently disarming every test using it. Strengthens three assertions that could not fail for the reason they name: the card-repair table had one survivor to choose from, the spin guard's bound was unreachable because re-entry dies of OOM first, and QuoteDisplay's two userInitiated call sites were unasserted.
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.
Every quote writer read the array, mutated it, and wrote it back — so a pull landing in the service worker between the read and the write was erased by whatever the page wrote. Quotes were the last collection without the lock goals, reminders and collections already had.
All 19 writers now read inside the
'quotes'web lock the sync engine'sarrayBindingalready takes, and collect their sync notify ids from that locked read — so a quote a pull removed is not marked dirty and tombstoned for every other device.Most of this PR is not the lock
Checking the write result made a lot of previously-silent failure reachable, and that exposed the reporting layer behind it. Fifteen review rounds turned up ~75 defects; these are the ones worth a reviewer's attention:
isCustomQuotecounts a favourited seed quote as custom, so the write moves it between storage keys — seed first, which drops it, then custom, which on a full disk never takes it. The quote ended up in neither key while the user was told only that the update failed. Pre-existing onmain; the seed key is now restored when the second write fails.initializetrusted the stored card's contents and checked only that its id still existed, so a quote another device hid or edited kept rendering here — no failure involved.erroras "the load failed", so clicking the heart while over quota replaced the card, the shortcut and the quotes page with a "Failed to load quotes" panel — including the page you would use to free space.AddQuoteFormandEditQuoteModalcleared and closed on a failed save; the collection and reset dialogs vanished on writes that had not landed.Notes for review
resetAllQuotesstill replaces the list wholesale; it reads only to learn which ids it destroys.persistOneQuotetakes its own lock and answers a discriminated union, so a write aimed at an id the locked read no longer holds is skipped rather than rewritten, and no caller can adopt an unpersisted list.Every guard is mutation-tested: the lock, the locked read, each write-failure guard, each gone branch, each card repair and each outcome the UI acts on has a named test that fails when the guard is removed.
Worth knowing before you read it: the card-repair path in
incrementViewCounttook five attempts across three rounds — an unbounded recursion that exhausted the heap, a re-entry guard that reopened the phantom it existed to close, a blanked card, and an unfiltered pick — before the current version. It is the piece I would read most closely.