Move the settings to localStorage and the doc indexes to IndexedDB - #2739
Merged
Merged
Conversation
Safari and Brave cap a script-written cookie at seven days, whatever expiry it asks for, so the enabled docs vanished on their own and the app wiped the offline data that went with them. The cookies are mirrored in localStorage now, and written back from it at boot. Fixes #1765
The service worker precached the enabled docs' index.json, so it had to be rendered per request from the docs cookie, and one missing index failed the whole install. The fetch handler caches index.json as it goes instead: the precache list is just the app shell, and the server reads no cookies.
Nothing server-side reads them any more, and cookies were the wrong place regardless. CookiesStore becomes SettingsStore, one JSON object in localStorage, which takes in whatever is still in the jar on first run and expires it. Analytics consent moves to the store, the once-a-session prompt to sessionStorage, and the mobile override to localStorage. The vendored Cookies.js is gone; only the analytics vendors' own cookies remain.
A few of the larger index files exhaust localStorage's ~5 MB quota, and LocalStorageStore swallows the QuotaExceededError, so the cache quietly stopped working. They move to an indexes store in the docs database, keyed by slug. DB.VERSION is unchanged: a database that predates the store bumps its own schema when it first misses it. Doc#load reads the cache asynchronously now, and a backup carries the index it reads back from the store.
The index migration is lazy: loadIndex takes in whatever localStorage still holds when it misses, so the boot no longer sweeps and reparses every cached index before first paint, and DB stops reaching into localStorage at all. One accessor hands out the indexes store, so the index methods lose their retry plumbing, and app.reload() clears the cache in a single transaction instead of one per doc.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect storage migration, cache clearing, and service-worker reliability.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
assets/javascripts/app/db.js:537
- Although
Doc#loaddocuments an asynchronous cache path (and the old implementation usedsetTimeout),db()invokes callbacks synchronously when IndexedDB is disabled, so this!reqbranch callsfnsynchronously for a legacy localStorage hit.Docs#loadthen recursively advances through cached docs, which can overflow or violate callback ordering in browsers without IndexedDB. Defer this callback to preserve the documented contract.
fn(this.importIndex(doc, mtime));
assets/javascripts/app/db.js:569
- The lazy migration intentionally leaves legacy per-doc index records in
app.localStorageuntil a doc is read, but this method only clears the new IndexedDB store. A hard reload can therefore import an untouched legacy index again on the next boot, despitereload()promising to drop every cached index. Clear the legacy per-doc keys as part of this transition or make the reload path account for them.
clearIndexes() {
this.indexes("readwrite", (store) => store?.clear());
}
assets/javascripts/lib/settings_store.js:88
App.reset()calls this method and previouslyCookiesStore.reset()also expired the session cookie used for analytics prompts. After this migration,analyticsConsentAskedlives in sessionStorage and survives a reset; the consent setting is removed but the prompt remains suppressed until the tab closes. Clear the session key as part of the full app reset.
this.storage.del(SettingsStore.KEY);
assets/javascripts/lib/settings_store.js:134
- When a user arrives with the old
analyticsConsentAsked=1session cookie, this branch simply omits it from the settings object and then expires it. It never seedssessionStorage, soconsentAsked()returns false on that same session and shows the consent prompt again after the migration. Copy this flag to sessionStorage before expiring the cookie.
// analyticsConsentAsked was a session cookie, and is sessionStorage now.
if (key !== "analyticsConsentAsked" && !(key in settings)) {
settings[key] = decode(value || "");
}
expireCookie(name);
assets/javascripts/models/doc.js:185
DB#loadIndexcan invoke this callback synchronously when IndexedDB is unavailable, becauseDB#dbcalls its callback directly andloadIndexfalls through toimportIndex.Docs#loadincrements its index only afterDoc#loadreturns, so a current localStorage hit re-enters the same document before that increment and recurses until the stack overflows. Keep the cache callback asynchronous, as the removed localStorage path did.
app.db.loadIndex(this, this.mtime, (data) => {
if (data) {
this.reset(data);
onSuccess();
} else {
views/service-worker.js.erb:53
- Because the request includes
?mtime, each update of a doc stores another response under a new Cache key whilecacheNameis based only on app assets; doc updates therefore do not activate the old-cache cleanup. Over time this can retain multiple multi-megabyte indexes per doc until CacheStorage quota is exhausted. Prune older entries for the same index pathname after storing the new one, or use a stable cache key and validate the mtime.
cache.put(event.request, response.clone());
- Files reviewed: 24/26 changed files
- Comments generated: 6
- Review effort level: Lite
| this.previousErrorHandler = onerror; | ||
| window.onerror = this.onWindowError.bind(this); | ||
| CookiesStore.onBlocked = this.onCookieBlocked; | ||
| SettingsStore.onBlocked = this.onStorageBlocked; |
| reload() { | ||
| this.docs.clearCache(); | ||
| this.disabledDocs.clearCache(); | ||
| this.db.clearIndexes(); |
| return; | ||
| } | ||
|
|
||
| app.localStorage.del(doc.slug); |
Comment on lines
+123
to
+137
| for (var cookie of document.cookie.split(/;\s?/)) { | ||
| if (cookie[0] === "_") { | ||
| continue; | ||
| } | ||
| const [name, value] = cookie.split("="); | ||
| const key = decode(name); | ||
|
|
||
| // analyticsConsentAsked was a session cookie, and is sessionStorage now. | ||
| if (key !== "analyticsConsentAsked" && !(key in settings)) { | ||
| settings[key] = decode(value || ""); | ||
| } | ||
| expireCookie(name); | ||
| } | ||
|
|
||
| this.storage.set(SettingsStore.KEY, settings); |
Comment on lines
+34
to
+36
| const override = app.localStorage.get("override-mobile-detect"); | ||
| if (override != null) { | ||
| return !!override; |
| response.ok | ||
| ) { | ||
| const cache = await caches.open(cacheName); | ||
| cache.put(event.request, response.clone()); |
DB#db runs its callback there and then when IndexedDB is unavailable, so loadIndex handed Doc#load a localStorage hit before load() had returned. Docs#load advances its index after the call, so the callback re-entered the same doc and recursed until the stack gave out.
SettingsStore#migrate folds the override-mobile-detect cookie into the settings object with everything else, but Mobile.detect looked for a localStorage key of its own that nothing ever wrote. The override was dropped on upgrade, and there was no way left to set it. The cookie held "true" or "false", which the store hands back as they stand, so both those and 1 / 0 are read.
The migration expired each cookie as it read it and wrote the settings afterwards, and LocalStorageStore reports a write it couldn't make rather than throwing. A browser that wouldn't take the write — private browsing, an exhausted quota — was left with neither copy, and the next boot came up on the defaults.
importIndex deleted the legacy value before writing it to the database, and the write can go nowhere: a database that predates the indexes store only queues its schema bump on the first miss, and a browser without IndexedDB has no store at all. The one cached copy was lost either way.
cache.put was neither awaited nor handed to waitUntil, so respondWith could settle and the worker be terminated before the index was written, leaving the doc unavailable offline after the load that was meant to cache it. The response still goes back without waiting on the write.
clearIndexes opened its transaction and returned, and app.reload() navigated away without waiting, so the clear a hard reload promises could be lost. It takes a callback now, and the reload hangs off it. It also sweeps the indexes the lazy migration has left in localStorage, which a reload would otherwise take straight back in.
analyticsConsentAsked was a session cookie and is sessionStorage now, but the migration only dropped the cookie, so the prompt came back on the visit that upgraded. A reset didn't clear it either, leaving the prompt suppressed for the rest of the tab's life. The key is the store's to name; page.js reads it from there.
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.
DevDocs would periodically forget everything — enabled docs back to the
defaults, offline data dropped with them. Not the update notification, as the
issue guesses: the settings were cookies written from JavaScript with
expires: 1e8(~3 years), and Safari and Brave cap a script-written cookie atseven days regardless. Once
docsexpired,Settings#getDocsfell back toconfig.default_docsandApp#saveDocs→DB#migratedropped the installeddocs.
The settings move to localStorage, as one JSON object under a
settingskey;the doc indexes move the other way, from localStorage into the existing
docsIndexedDB database. Nothing the app owns is a cookie any more.
The server stops reading cookies
/service-worker.jswas the last reader — it baked the enabled docs'index.jsonURLs intourlsToCache. The fetch handler cachesindex.jsonasit goes instead, so the precache list is the app shell alone, and one 404ing
index no longer fails the install (
cache.addAllis all-or-nothing). Removesdocs,memoized_cookies,user_has_docs?anddoc_index_urlsfromlib/app.rb.Settings → localStorage
CookiesStorebecomesSettingsStore, which takes in the existing cookies onfirst run and expires them. Consent moves into the store, the once-a-session
consent flag to
sessionStorage(it was a session cookie), the mobile-detectoverride to localStorage.
Cookies.jsis deleted; the only cookies left are theanalytics vendors' own, still cleared by
resetAnalytics.Doc indexes → IndexedDB
Into an
indexesstore keyed by slug. Rust'sindex.jsonis 4.1 MB on its own,against a ~5 MB origin quota — and
LocalStorageStore.setswallows theQuotaExceededError, so the cache had been failing silently.DB.VERSIONstays15: a database predating the store bumps its own schema on the first
NotFoundError. Indexes still in localStorage are taken in lazily, on the readthat wants them.
Testing
npm test(28),npm run typecheck,bundle exec rake(795).The IndexedDB paths have never run in a browser — Node has no IndexedDB, so
the tests stub the store. Worth manual verification before merge.
Two rough edges, both introduced here: the service worker keeps one
index.jsonper doc build (?mtimeis part of the cache key) with no pruningbetween deploys; and a boot costs about one
indexedDB.open()per enabled doc,since
DB#dbcloses after each synchronous batch whileDocs#loadadvancesfrom inside the IDB callback.
Fixes #1765