From 5e84f60069873d7d5fa77ee7574eec5e62745507 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 16:01:35 +0200 Subject: [PATCH 01/12] Mirror the settings cookies in localStorage 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 --- assets/javascripts/app/settings.js | 3 +- assets/javascripts/lib/cookies_store.js | 166 +++++++++++++-- .../javascripts/templates/pages/about_tmpl.js | 2 +- test/assets/cookies_store_test.js | 191 ++++++++++++++++++ 4 files changed, 339 insertions(+), 23 deletions(-) create mode 100644 test/assets/cookies_store_test.js diff --git a/assets/javascripts/app/settings.js b/assets/javascripts/app/settings.js index 9cdf963c75..f2cdd9ac4c 100644 --- a/assets/javascripts/app/settings.js +++ b/assets/javascripts/app/settings.js @@ -47,7 +47,8 @@ import { $ } from "../lib/util.js"; */ /** - * The user's preferences, stored in cookies so that the server can read them. + * The user's preferences, stored in cookies so that the server can read them, + * and mirrored in localStorage so that they outlive the cookies. * * `PREFERENCE_KEYS` are the ones the user controls and that a backup carries; * `INTERNAL_KEYS` are the app's own bookkeeping and stay out of backups. diff --git a/assets/javascripts/lib/cookies_store.js b/assets/javascripts/lib/cookies_store.js index a9580202f7..3decb3bab2 100644 --- a/assets/javascripts/lib/cookies_store.js +++ b/assets/javascripts/lib/cookies_store.js @@ -1,5 +1,7 @@ // @ts-check +import { LocalStorageStore } from "./local_storage_store.js"; + /** * A cookie-backed key/value store. * @@ -8,6 +10,14 @@ * cause is the browser blocking cookies — `onBlocked` is called so the app can * warn the user. * + * Every value is also mirrored in localStorage, because cookies alone don't + * last: Safari and Brave cap a cookie written from a script to seven days + * however far ahead its expiry date is set, and a browser under pressure may + * evict one sooner. Losing the cookies loses the enabled docs with them, and + * the app then wipes the offline data that went with them, so the mirror is + * copied back over the cookies on the next visit. + * Related issue: https://github.com/freeCodeCamp/devdocs/issues/1765 + * * Intentionally called CookiesStore instead of CookieStore. Calling it * CookieStore causes issues when the Experimental Web Platform features flag is * enabled in Chrome. @@ -18,6 +28,9 @@ export class CookiesStore { static INT = /^\d+$/; + /** The localStorage key the cookies are mirrored under. */ + static MIRROR_KEY = "settings"; + /** * Hook called when a value read back after a write doesn't match what was * written. Replaced by the app at boot; a no-op by default. @@ -28,17 +41,61 @@ export class CookiesStore { */ static onBlocked(key, value, actual) {} + /** + * @param {CookieValue} value + * @returns {CookieValue} `value`, as a number when it is all digits. + */ + static parse(value) { + return value != null && CookiesStore.INT.test(String(value)) + ? parseInt(String(value), 10) + : value; + } + + /** + * Cookies travel percent-encoded, and `Cookies.set` encodes what it is + * given, so a value read straight off `document.cookie` has to be decoded + * before it can be written back or it gains a round of escaping each time. + * + * @param {string} value + * @returns {string} `value`, decoded, or as it stands when it isn't valid + * percent-encoding. + */ + static decode(value) { + try { + return decodeURIComponent(value); + } catch (error) { + return value; + } + } + + /** + * Writes a cookie with the lifetime the app asks for. How much of that the + * browser honours is up to it — hence the mirror. + * + * @param {string} key + * @param {string} value + */ + static writeCookie(key, value) { + Cookies.set(key, value, { path: "/", expires: 1e8 }); + } + + /** Opens the mirror and puts back the cookies the browser has dropped. */ + constructor() { + this.storage = new LocalStorageStore(); + this.mirrored = this.restore(); + } + /** * @param {string} key * @returns {CookieValue} The stored value, as a number when it is all digits. */ get(key) { - /** @type {CookieValue} */ - let value = Cookies.get(key); - if (value != null && CookiesStore.INT.test(value)) { - value = parseInt(value, 10); - } - return value; + // The mirror answers first: it is seeded from the cookies at boot and kept + // in step with every write afterwards, so it holds what the app last + // stored even when the browser refused the cookie — a cookie the user's + // list of docs has outgrown, typically. + const value = this.mirrored[key]; + return CookiesStore.parse(value != null ? value : Cookies.get(key)); } /** @@ -56,27 +113,28 @@ export class CookiesStore { if (value === true) { value = 1; } - if ( - value && - (typeof CookiesStore.INT.test === "function" - ? CookiesStore.INT.test(/** @type {string} */ (value)) - : undefined) - ) { - value = parseInt(/** @type {string} */ (value), 10); - } - Cookies.set(key, "" + value, { path: "/", expires: 1e8 }); - if (this.get(key) !== value) { - CookiesStore.onBlocked(key, value, this.get(key)); + value = CookiesStore.parse(value); + + this.save(key, "" + value); + + // Read the cookie itself rather than going through `get`, so that a value + // the mirror is holding on to doesn't hide a cookie the browser refused. + const actual = CookiesStore.parse(Cookies.get(key)); + if (actual !== value) { + CookiesStore.onBlocked(key, value, actual); } } /** @param {string} key */ del(key) { Cookies.expire(key); + this.save(key, undefined); } - /** Expires every cookie on the document. */ + /** Expires every cookie on the document, and empties the mirror. */ reset() { + this.mirrored = {}; + this.storage.del(CookiesStore.MIRROR_KEY); try { for (var cookie of document.cookie.split(/;\s?/)) { Cookies.expire(cookie.split("=")[0]); @@ -86,16 +144,82 @@ export class CookiesStore { } /** - * @returns {Record} Every non-internal cookie, unparsed. + * @returns {Record} Everything the store holds, unparsed. */ dump() { + return { ...this.mirrored }; + } + + /** + * @returns {Record} Every non-internal cookie on the + * document, decoded but otherwise unparsed. + */ + cookies() { const result = {}; for (var cookie of document.cookie.split(/;\s?/)) { - if (cookie[0] !== "_") { + if (cookie && cookie[0] !== "_") { const [name, value] = cookie.split("="); - result[name] = value; + result[CookiesStore.decode(name)] = CookiesStore.decode(value || ""); } } return result; } + + /** + * Stores `key` in the cookies and in the mirror, or deletes it from both + * when `value` is `undefined`. + * + * @param {string} key + * @param {string | undefined} value + */ + save(key, value) { + const stored = this.read(); + + if (value !== undefined) { + CookiesStore.writeCookie(key, value); + this.mirrored[key] = stored[key] = value; + } else { + delete this.mirrored[key]; + delete stored[key]; + } + + // What is on disk is changed a key at a time, so that a setting another + // tab has written since this one read the mirror isn't rolled back. + this.storage.set(CookiesStore.MIRROR_KEY, stored); + } + + /** + * Takes the mirror as the record of what is stored, and writes back the + * cookies that have since gone missing. + * + * Cookies that are still there win, and seed the mirror: they are what the + * server has just been told, they are what another tab may have changed in + * the meantime, and they cover the visitors whose settings predate the + * mirror. + * + * @returns {Record} The mirrored values. + */ + restore() { + const mirrored = { ...this.read(), ...this.cookies() }; + + for (var key in mirrored) { + if (Cookies.get(key) === undefined) { + CookiesStore.writeCookie(key, mirrored[key]); + } + } + + this.storage.set(CookiesStore.MIRROR_KEY, mirrored); + return mirrored; + } + + /** + * @returns {Record} The mirror, or an empty object when it is + * missing or unreadable. + */ + read() { + const mirrored = this.storage.get(CookiesStore.MIRROR_KEY); + return mirrored && typeof mirrored === "object" + ? /** @type {Record} */ ({ ...mirrored }) + : {}; + } } diff --git a/assets/javascripts/templates/pages/about_tmpl.js b/assets/javascripts/templates/pages/about_tmpl.js index a57957cc45..a2d82ee9f6 100644 --- a/assets/javascripts/templates/pages/about_tmpl.js +++ b/assets/javascripts/templates/pages/about_tmpl.js @@ -91,7 +91,7 @@ export const aboutPage = function () {
  • We do not collect personal information through the app.
  • We use Google Analytics and Gauges to collect anonymous traffic information if you have given consent to this. You can change your decision in the settings.
  • We use Sentry to collect crash data and improve the app. -
  • The app uses cookies to store user preferences. +
  • The app uses cookies and local storage to store user preferences.
  • By using the app, you signify your acceptance of this policy. If you do not agree to this policy, please do not use the app.
  • If you have any questions regarding privacy, please email privacy@freecodecamp.org. \ diff --git a/test/assets/cookies_store_test.js b/test/assets/cookies_store_test.js new file mode 100644 index 0000000000..5ea1a03607 --- /dev/null +++ b/test/assets/cookies_store_test.js @@ -0,0 +1,191 @@ +// @ts-check + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { CookiesStore } from "../../assets/javascripts/lib/cookies_store.js"; + +// A cookie jar the vendored Cookies.js can write to: keys and values stay +// percent-encoded, as they are on a real `document.cookie`, and a write whose +// expiry has passed deletes the key. +/** @type {Map} */ +const jar = new Map(); +let cookiesAccepted = true; + +Object.defineProperty(document, "cookie", { + get: () => + [...jar].map(([key, value]) => `${key}=${value}`).join("; "), + set: (string) => { + if (!cookiesAccepted) return; + const [pair, ...attributes] = string.split(";"); + const separator = pair.indexOf("="); + const key = pair.slice(0, separator); + const expires = attributes + .map((attribute) => attribute.trim()) + .find((attribute) => /^expires=/i.test(attribute)); + + if (expires && new Date(expires.slice(8)) <= new Date()) { + jar.delete(key); + } else { + jar.set(key, pair.slice(separator + 1)); + } + }, + configurable: true, +}); + +/** @type {Map} */ +const storage = new Map(); + +Object.defineProperty(globalThis, "localStorage", { + value: { + getItem: (/** @type {string} */ key) => + storage.has(key) ? storage.get(key) : null, + setItem: (/** @type {string} */ key, /** @type {string} */ value) => + storage.set(key, String(value)), + removeItem: (/** @type {string} */ key) => storage.delete(key), + clear: () => storage.clear(), + }, + writable: true, + configurable: true, +}); + +// The store reads the vendored library off the global, and the library binds +// itself to whatever `window` it is loaded with, so hand it the document above +// before it evaluates. +// @ts-ignore -- the fake window from setup.js has no document until now. +window.document = document; +await import("../../assets/javascripts/vendor/cookies.js"); +Object.defineProperty(globalThis, "Cookies", { + // @ts-ignore -- where the library puts itself under Node. + value: window.Cookies, + writable: true, + configurable: true, +}); + +/** Wipes both stores, as a fresh browser profile would. */ +const reset = () => { + jar.clear(); + storage.clear(); + cookiesAccepted = true; +}; + +/** Drops the cookies while leaving localStorage alone, as Safari's seven-day cap does. */ +const expireCookies = () => jar.clear(); + +test("stores and reads back values, parsing the integers", () => { + reset(); + const store = new CookiesStore(); + + store.set("docs", "css/javascript"); + store.set("size", 320); + store.set("hideIntro", true); + + assert.equal(store.get("docs"), "css/javascript"); + assert.equal(store.get("size"), 320); + assert.equal(store.get("hideIntro"), 1); + assert.equal(store.get("missing"), undefined); +}); + +test("puts back the cookies the browser has dropped", () => { + reset(); + new CookiesStore().set("docs", "css/javascript"); + + expireCookies(); + const store = new CookiesStore(); + + assert.equal(store.get("docs"), "css/javascript"); + assert.match(document.cookie, /docs=css\/javascript/); +}); + +test("puts back the settings of a visitor who has no mirror yet", () => { + reset(); + // The cookie is there, but predates the mirror, as on the visit that first + // runs this version of the app. + document.cookie = "docs=css/javascript"; + new CookiesStore(); + + expireCookies(); + assert.equal(new CookiesStore().get("docs"), "css/javascript"); +}); + +test("keeps a value intact when it is restored, rather than re-escaping it", () => { + reset(); + new CookiesStore().set("layout", "_max-width _sidebar-hidden"); + + expireCookies(); + const store = new CookiesStore(); + + assert.equal(store.get("layout"), "_max-width _sidebar-hidden"); + assert.deepEqual(store.dump(), { layout: "_max-width _sidebar-hidden" }); +}); + +test("does not put back a deleted value", () => { + reset(); + const store = new CookiesStore(); + store.set("docs", "css/javascript"); + store.del("docs"); + + assert.equal(store.get("docs"), undefined); + + expireCookies(); + assert.equal(new CookiesStore().get("docs"), undefined); +}); + +test("does not put anything back after a reset", () => { + reset(); + const store = new CookiesStore(); + store.set("docs", "css/javascript"); + store.set("size", 320); + store.reset(); + + assert.equal(store.get("docs"), undefined); + assert.deepEqual(new CookiesStore().dump(), {}); +}); + +test("falls back to the mirror, and still reports the block, when cookies are refused", () => { + reset(); + const store = new CookiesStore(); + /** @type {unknown[]} */ + const blocked = []; + const onBlocked = CookiesStore.onBlocked; + CookiesStore.onBlocked = (key, value, actual) => + blocked.push([key, value, actual]); + + try { + cookiesAccepted = false; + store.set("docs", "css/javascript"); + + assert.deepEqual(blocked, [["docs", "css/javascript", undefined]]); + assert.equal(store.get("docs"), "css/javascript"); + assert.equal(new CookiesStore().get("docs"), "css/javascript"); + } finally { + CookiesStore.onBlocked = onBlocked; + } +}); + +test("keeps a setting another tab has written since the mirror was read", () => { + reset(); + const tabA = new CookiesStore(); + const tabB = new CookiesStore(); + + tabA.set("theme", "dark"); + tabB.set("size", 320); + + expireCookies(); + const store = new CookiesStore(); + + assert.equal(store.get("theme"), "dark"); + assert.equal(store.get("size"), 320); +}); + +test("keeps reading a value whose cookie the browser has refused", () => { + reset(); + const store = new CookiesStore(); + store.set("docs", "css"); + + cookiesAccepted = false; + store.set("docs", "css/javascript"); + + assert.equal(store.get("docs"), "css/javascript"); + assert.deepEqual(store.dump(), { docs: "css/javascript" }); +}); From bc51d4c5866b5ebcd446131d18ba5d76e1eb11ba Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 16:07:24 +0200 Subject: [PATCH 02/12] Cache the doc indexes as they are fetched 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. --- lib/app.rb | 31 ------------------------------- views/service-worker.js.erb | 17 ++++++++++++++++- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/lib/app.rb b/lib/app.rb index 3c74344136..aee80ccece 100644 --- a/lib/app.rb +++ b/lib/app.rb @@ -169,26 +169,10 @@ def self.parse_news helpers do include Sprockets::Helpers - def memoized_cookies - @memoized_cookies ||= request.cookies - end - def canonical_origin "https://#{request.host_with_port}" end - def docs - @docs ||= begin - cookie = memoized_cookies['docs'] - - if cookie.nil? - settings.default_docs - else - cookie.split('/') - end - end - end - def find_doc(slug) settings.docs[slug] || begin settings.docs.each do |_, doc| @@ -198,21 +182,6 @@ def find_doc(slug) end end - def user_has_docs?(slug) - docs.include?(slug) || begin - slug = "#{slug}~" - docs.any? { |_slug| _slug.start_with?(slug) } - end - end - - def doc_index_urls - docs.each_with_object [] do |slug, result| - if doc = settings.docs[slug] - result << "#{settings.docs_origin}/#{slug}/index.json?#{doc['mtime']}" - end - end - end - def doc_index_page? @doc && (request.path == "/#{@doc['slug']}/" || request.path == "/#{@doc['slug_without_version']}/") end diff --git a/views/service-worker.js.erb b/views/service-worker.js.erb index 81689a2efb..d6fe4b493c 100644 --- a/views/service-worker.js.erb +++ b/views/service-worker.js.erb @@ -8,7 +8,6 @@ const urlsToCache = [ '/favicon.ico', '/manifest.json', '<%= service_worker_asset_urls.join "',\n '" %>', - '<%= doc_index_urls.join "',\n '" %>', ]; <%# Set-up the cache %> @@ -37,6 +36,22 @@ self.addEventListener('fetch', event => { try { const response = await fetch(event.request); + + <%# Cache documentation index files as they are fetched, so that enabled %> + <%# docs stay available offline without baking the (per-user) list of %> + <%# enabled docs into this file at request time. index.json is served %> + <%# from the docs CDN (a different origin in production), which sends CORS %> + <%# headers, so the response is inspectable and cacheable — no origin check. %> + const url = new URL(event.request.url); + if ( + event.request.method === 'GET' && + url.pathname.endsWith('/index.json') && + response.ok + ) { + const cache = await caches.open(cacheName); + cache.put(event.request, response.clone()); + } + return response; } catch (err) { const url = new URL(event.request.url); From 7f0e4d8e417d73b5da652fd6a7ad05968dd72a44 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 16:23:13 +0200 Subject: [PATCH 03/12] Store the settings in localStorage instead of 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. --- assets/javascripts/app/app.js | 18 +- assets/javascripts/app/settings.js | 11 +- assets/javascripts/globals.d.ts | 18 -- assets/javascripts/lib/cookies_store.js | 225 ------------------ assets/javascripts/lib/page.js | 33 ++- assets/javascripts/lib/settings_store.js | 168 +++++++++++++ assets/javascripts/templates/error_tmpl.js | 4 +- assets/javascripts/templates/notif_tmpl.js | 6 +- .../javascripts/templates/pages/about_tmpl.js | 2 +- assets/javascripts/tracking.js | 3 +- assets/javascripts/vendor.js | 2 +- assets/javascripts/vendor/cookies.js | 208 ---------------- .../javascripts/views/content/offline_page.js | 4 +- assets/javascripts/views/layout/document.js | 6 +- assets/javascripts/views/layout/mobile.js | 5 +- test/app_test.rb | 25 +- test/assets/cookies_store_test.js | 191 --------------- test/assets/settings_store_test.js | 166 +++++++++++++ test/assets/setup.js | 1 + 19 files changed, 396 insertions(+), 700 deletions(-) delete mode 100644 assets/javascripts/lib/cookies_store.js create mode 100644 assets/javascripts/lib/settings_store.js delete mode 100644 assets/javascripts/vendor/cookies.js delete mode 100644 test/assets/cookies_store_test.js create mode 100644 test/assets/settings_store_test.js diff --git a/assets/javascripts/app/app.js b/assets/javascripts/app/app.js index 60c2d58977..2c52255026 100644 --- a/assets/javascripts/app/app.js +++ b/assets/javascripts/app/app.js @@ -9,7 +9,7 @@ import { Shortcuts } from "./shortcuts.js"; import { UpdateChecker } from "./update_checker.js"; import { Docs } from "../collections/docs.js"; import { Entries } from "../collections/entries.js"; -import { CookiesStore } from "../lib/cookies_store.js"; +import { SettingsStore } from "../lib/settings_store.js"; import { Events } from "../lib/events.js"; import { LocalStorageStore } from "../lib/local_storage_store.js"; import { $ } from "../lib/util.js"; @@ -166,7 +166,7 @@ export class App extends Events { } this.previousErrorHandler = onerror; window.onerror = this.onWindowError.bind(this); - CookiesStore.onBlocked = this.onCookieBlocked; + SettingsStore.onBlocked = this.onStorageBlocked; } } @@ -477,19 +477,19 @@ export class App extends Events { } /** - * Warns the user that cookies are blocked, so preferences won't stick. Once. + * Warns the user that storage is blocked, so preferences won't stick. Once. * * @param {string} key * @param {unknown} value What was written. * @param {unknown} actual What was read back. */ - onCookieBlocked(key, value, actual) { - if (this.cookieBlocked) { + onStorageBlocked(key, value, actual) { + if (this.storageBlocked) { return; } - this.cookieBlocked = true; - new Notif("CookieBlocked", { autoHide: null }); - Raven.captureMessage(`CookieBlocked/${key}`, { + this.storageBlocked = true; + new Notif("StorageBlocked", { autoHide: null }); + Raven.captureMessage(`StorageBlocked/${key}`, { level: "warning", extra: { value, actual }, }); @@ -497,7 +497,7 @@ export class App extends Events { /** @param {...unknown} args The `window.onerror` arguments. */ onWindowError(...args) { - if (this.cookieBlocked) { + if (this.storageBlocked) { return; } if (this.isAppError(args[0], /** @type {string} */ (args[1]))) { diff --git a/assets/javascripts/app/settings.js b/assets/javascripts/app/settings.js index f2cdd9ac4c..9d1d0b5db8 100644 --- a/assets/javascripts/app/settings.js +++ b/assets/javascripts/app/settings.js @@ -2,13 +2,13 @@ import { app } from "./app.js"; import { config } from "./config.js"; -import { CookiesStore } from "../lib/cookies_store.js"; +import { settingsStore } from "../lib/settings_store.js"; import { $ } from "../lib/util.js"; /** * A setting the user turns on or off. * - * It is a boolean going in, but CookiesStore writes `true` as `1` and parses + * It is a boolean going in, but SettingsStore writes `true` as `1` and parses * the digit back out on read, so it comes back as a number. A setting that was * never written falls back to its default, which is a real boolean. Both are * truthy or falsy as intended; only a strict comparison would go wrong. @@ -47,8 +47,7 @@ import { $ } from "../lib/util.js"; */ /** - * The user's preferences, stored in cookies so that the server can read them, - * and mirrored in localStorage so that they outlive the cookies. + * The user's preferences, stored in localStorage. * * `PREFERENCE_KEYS` are the ones the user controls and that a backup carries; * `INTERNAL_KEYS` are the app's own bookkeeping and stay out of backups. @@ -100,9 +99,9 @@ export class Settings { autoLatestVersion: false, }; - /** Opens the cookie store and starts following the system colour scheme. */ + /** Opens the store and starts following the system colour scheme. */ constructor() { - this.store = new CookiesStore(); + this.store = settingsStore; this.cache = {}; this.autoSupported = window.matchMedia("(prefers-color-scheme)").media !== "not all"; diff --git a/assets/javascripts/globals.d.ts b/assets/javascripts/globals.d.ts index 40cf8cd5ca..eb5036f9e4 100644 --- a/assets/javascripts/globals.d.ts +++ b/assets/javascripts/globals.d.ts @@ -16,24 +16,6 @@ export {}; declare global { // --- Vendored libraries (assets/javascripts/vendor) --- - /** Cookies.js — github.com/ScottHamper/Cookies */ - const Cookies: { - (key: string): string | undefined; - (key: string, value: string, options?: CookieOptions): typeof Cookies; - get(key: string): string | undefined; - set(key: string, value: string, options?: CookieOptions): typeof Cookies; - expire(key: string, options?: CookieOptions): typeof Cookies; - defaults: CookieOptions; - enabled: boolean; - }; - - interface CookieOptions { - path?: string; - domain?: string; - expires?: number | string | Date; - secure?: boolean; - } - /** Raven.js — the Sentry browser client. Only the parts the app uses. */ const Raven: { config(dsn: string, options?: Record): typeof Raven; diff --git a/assets/javascripts/lib/cookies_store.js b/assets/javascripts/lib/cookies_store.js deleted file mode 100644 index 3decb3bab2..0000000000 --- a/assets/javascripts/lib/cookies_store.js +++ /dev/null @@ -1,225 +0,0 @@ -// @ts-check - -import { LocalStorageStore } from "./local_storage_store.js"; - -/** - * A cookie-backed key/value store. - * - * Values round-trip as strings, so integers are parsed back out on read and - * booleans are stored as `1` / absent. When a write doesn't stick — the usual - * cause is the browser blocking cookies — `onBlocked` is called so the app can - * warn the user. - * - * Every value is also mirrored in localStorage, because cookies alone don't - * last: Safari and Brave cap a cookie written from a script to seven days - * however far ahead its expiry date is set, and a browser under pressure may - * evict one sooner. Losing the cookies loses the enabled docs with them, and - * the app then wipes the offline data that went with them, so the mirror is - * copied back over the cookies on the next visit. - * Related issue: https://github.com/freeCodeCamp/devdocs/issues/1765 - * - * Intentionally called CookiesStore instead of CookieStore. Calling it - * CookieStore causes issues when the Experimental Web Platform features flag is - * enabled in Chrome. - * Related issue: https://github.com/freeCodeCamp/devdocs/issues/932 - * - * @typedef {string | number | undefined} CookieValue - */ -export class CookiesStore { - static INT = /^\d+$/; - - /** The localStorage key the cookies are mirrored under. */ - static MIRROR_KEY = "settings"; - - /** - * Hook called when a value read back after a write doesn't match what was - * written. Replaced by the app at boot; a no-op by default. - * - * @param {string} key - * @param {CookieValue | boolean} value The value that was written. - * @param {CookieValue} actual The value that was read back. - */ - static onBlocked(key, value, actual) {} - - /** - * @param {CookieValue} value - * @returns {CookieValue} `value`, as a number when it is all digits. - */ - static parse(value) { - return value != null && CookiesStore.INT.test(String(value)) - ? parseInt(String(value), 10) - : value; - } - - /** - * Cookies travel percent-encoded, and `Cookies.set` encodes what it is - * given, so a value read straight off `document.cookie` has to be decoded - * before it can be written back or it gains a round of escaping each time. - * - * @param {string} value - * @returns {string} `value`, decoded, or as it stands when it isn't valid - * percent-encoding. - */ - static decode(value) { - try { - return decodeURIComponent(value); - } catch (error) { - return value; - } - } - - /** - * Writes a cookie with the lifetime the app asks for. How much of that the - * browser honours is up to it — hence the mirror. - * - * @param {string} key - * @param {string} value - */ - static writeCookie(key, value) { - Cookies.set(key, value, { path: "/", expires: 1e8 }); - } - - /** Opens the mirror and puts back the cookies the browser has dropped. */ - constructor() { - this.storage = new LocalStorageStore(); - this.mirrored = this.restore(); - } - - /** - * @param {string} key - * @returns {CookieValue} The stored value, as a number when it is all digits. - */ - get(key) { - // The mirror answers first: it is seeded from the cookies at boot and kept - // in step with every write afterwards, so it holds what the app last - // stored even when the browser refused the cookie — a cookie the user's - // list of docs has outgrown, typically. - const value = this.mirrored[key]; - return CookiesStore.parse(value != null ? value : Cookies.get(key)); - } - - /** - * Writing `false` deletes the key; `true` is stored as `1`. - * - * @param {string} key - * @param {CookieValue | boolean} value - */ - set(key, value) { - if (value === false) { - this.del(key); - return; - } - - if (value === true) { - value = 1; - } - value = CookiesStore.parse(value); - - this.save(key, "" + value); - - // Read the cookie itself rather than going through `get`, so that a value - // the mirror is holding on to doesn't hide a cookie the browser refused. - const actual = CookiesStore.parse(Cookies.get(key)); - if (actual !== value) { - CookiesStore.onBlocked(key, value, actual); - } - } - - /** @param {string} key */ - del(key) { - Cookies.expire(key); - this.save(key, undefined); - } - - /** Expires every cookie on the document, and empties the mirror. */ - reset() { - this.mirrored = {}; - this.storage.del(CookiesStore.MIRROR_KEY); - try { - for (var cookie of document.cookie.split(/;\s?/)) { - Cookies.expire(cookie.split("=")[0]); - } - return; - } catch (error) {} - } - - /** - * @returns {Record} Everything the store holds, unparsed. - */ - dump() { - return { ...this.mirrored }; - } - - /** - * @returns {Record} Every non-internal cookie on the - * document, decoded but otherwise unparsed. - */ - cookies() { - const result = {}; - for (var cookie of document.cookie.split(/;\s?/)) { - if (cookie && cookie[0] !== "_") { - const [name, value] = cookie.split("="); - result[CookiesStore.decode(name)] = CookiesStore.decode(value || ""); - } - } - return result; - } - - /** - * Stores `key` in the cookies and in the mirror, or deletes it from both - * when `value` is `undefined`. - * - * @param {string} key - * @param {string | undefined} value - */ - save(key, value) { - const stored = this.read(); - - if (value !== undefined) { - CookiesStore.writeCookie(key, value); - this.mirrored[key] = stored[key] = value; - } else { - delete this.mirrored[key]; - delete stored[key]; - } - - // What is on disk is changed a key at a time, so that a setting another - // tab has written since this one read the mirror isn't rolled back. - this.storage.set(CookiesStore.MIRROR_KEY, stored); - } - - /** - * Takes the mirror as the record of what is stored, and writes back the - * cookies that have since gone missing. - * - * Cookies that are still there win, and seed the mirror: they are what the - * server has just been told, they are what another tab may have changed in - * the meantime, and they cover the visitors whose settings predate the - * mirror. - * - * @returns {Record} The mirrored values. - */ - restore() { - const mirrored = { ...this.read(), ...this.cookies() }; - - for (var key in mirrored) { - if (Cookies.get(key) === undefined) { - CookiesStore.writeCookie(key, mirrored[key]); - } - } - - this.storage.set(CookiesStore.MIRROR_KEY, mirrored); - return mirrored; - } - - /** - * @returns {Record} The mirror, or an empty object when it is - * missing or unreadable. - */ - read() { - const mirrored = this.storage.get(CookiesStore.MIRROR_KEY); - return mirrored && typeof mirrored === "object" - ? /** @type {Record} */ ({ ...mirrored }) - : {}; - } -} diff --git a/assets/javascripts/lib/page.js b/assets/javascripts/lib/page.js index 7996ba4097..afbaf17544 100644 --- a/assets/javascripts/lib/page.js +++ b/assets/javascripts/lib/page.js @@ -1,5 +1,6 @@ import { app } from "../app/app.js"; import { config } from "../app/config.js"; +import { settingsStore } from "./settings_store.js"; import { $ } from "./util.js"; import { Notif } from "../views/misc/notif.js"; @@ -527,27 +528,41 @@ var track = function () { return; } - const consentGiven = Cookies.get("analyticsConsent"); - const consentAsked = Cookies.get("analyticsConsentAsked"); + const consentGiven = settingsStore.get("analyticsConsent"); - if (consentGiven === "1") { + if (consentGiven === 1) { for (var tracker of trackers) { tracker.call(undefined); } - } else if (consentGiven === undefined && consentAsked === undefined) { - // Only ask for consent once per browser session - Cookies.set("analyticsConsentAsked", "1"); - + } else if (consentGiven === undefined && !consentAsked()) { new Notif("AnalyticsConsent", { autoHide: null }); } }; -/** Expires the analytics cookies, which are the ones prefixed with a single `_`. */ +/** + * @returns {boolean} Whether the user has been asked for consent already, and + * marks them as asked if not. Kept in sessionStorage, so the question comes + * back on the next visit but not on the next page load. + */ +var consentAsked = function () { + try { + if (sessionStorage.getItem("analyticsConsentAsked")) { + return true; + } + sessionStorage.setItem("analyticsConsentAsked", "1"); + } catch (error) {} + return false; +}; + +/** + * Expires the analytics cookies, which are the ones the vendors set with a + * single leading `_`. The app has none of its own. + */ export const resetAnalytics = function () { for (var cookie of document.cookie.split(/;\s?/)) { var name = cookie.split("=")[0]; if (name[0] === "_" && name[1] !== "_") { - Cookies.expire(name); + document.cookie = `${name}=;path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT`; } } }; diff --git a/assets/javascripts/lib/settings_store.js b/assets/javascripts/lib/settings_store.js new file mode 100644 index 0000000000..d2299bab84 --- /dev/null +++ b/assets/javascripts/lib/settings_store.js @@ -0,0 +1,168 @@ +// @ts-check + +import { LocalStorageStore } from "./local_storage_store.js"; + +/** + * The user's settings, kept as one JSON object in localStorage. + * + * Values round-trip as strings, so integers are parsed back out on read and + * booleans are stored as `1` / absent — the shape the settings backup file has + * always had. When a write doesn't stick — storage turned off, private + * browsing, an exhausted quota — `onBlocked` is called so the app can warn the + * user. + * + * The settings used to be cookies, so that the server could read the enabled + * docs and render a service worker that precached them. Nothing server-side + * reads them any more, and cookies were the wrong place regardless: Safari and + * Brave cap a cookie written from a script at seven days however far ahead its + * expiry is set, so the settings quietly reset themselves — and the app wiped + * the offline data that went with them. + * Related issue: https://github.com/freeCodeCamp/devdocs/issues/1765 + * + * @typedef {string | number | undefined} SettingValue + */ +export class SettingsStore { + /** The localStorage key everything is stored under. */ + static KEY = "settings"; + + static INT = /^\d+$/; + + /** + * Hook called when a value read back after a write doesn't match what was + * written. Replaced by the app at boot; a no-op by default. + * + * @param {string} key + * @param {SettingValue | boolean} value The value that was written. + * @param {SettingValue} actual The value that was read back. + */ + static onBlocked(key, value, actual) {} + + /** + * @param {SettingValue} value + * @returns {SettingValue} `value`, as a number when it is all digits. + */ + static parse(value) { + return value != null && SettingsStore.INT.test(String(value)) + ? parseInt(String(value), 10) + : value; + } + + /** Opens the store and takes in whatever is still in cookies. */ + constructor() { + this.storage = new LocalStorageStore(); + this.migrate(); + } + + /** + * @param {string} key + * @returns {SettingValue} The stored value, as a number when it is all digits. + */ + get(key) { + return SettingsStore.parse(this.dump()[key]); + } + + /** + * Writing `false` deletes the key; `true` is stored as `1`. + * + * @param {string} key + * @param {SettingValue | boolean} value + */ + set(key, value) { + if (value === false) { + this.del(key); + return; + } + + if (value === true) { + value = 1; + } + value = SettingsStore.parse(value); + + const settings = this.dump(); + settings[key] = "" + value; + this.write(settings); + + const actual = this.get(key); + if (actual !== value) { + SettingsStore.onBlocked(key, value, actual); + } + } + + /** @param {string} key */ + del(key) { + const settings = this.dump(); + delete settings[key]; + this.write(settings); + } + + /** Clears every setting. */ + reset() { + this.storage.del(SettingsStore.KEY); + } + + /** + * @returns {Record} Every setting, unparsed. Read back out of + * storage each time, so that a second tab's writes aren't held stale. + */ + dump() { + const settings = this.storage.get(SettingsStore.KEY); + return settings && typeof settings === "object" + ? /** @type {Record} */ (settings) + : {}; + } + + /** @param {Record} settings */ + write(settings) { + this.storage.set(SettingsStore.KEY, settings); + } + + /** + * Takes in the settings an earlier version of the app left in cookies, and + * expires them. Does nothing once they are gone. + * + * What is already stored wins, being the newer of the two. Cookies with a + * single leading underscore belong to the analytics vendors, not to us. + */ + migrate() { + const settings = this.dump(); + let found = false; + + // Reading document.cookie throws where cookies are turned off entirely. + try { + for (var cookie of document.cookie.split(/;\s?/)) { + if (!cookie || 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 || ""); + found = true; + } + document.cookie = `${name}=;path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT`; + } + } catch (error) {} + + if (found) { + this.write(settings); + } + } +} + +/** + * @param {string} value + * @returns {string} `value`, decoded, or as it stands when it isn't valid + * percent-encoding. Cookies travelled percent-encoded; the store doesn't. + */ +const decode = (value) => { + try { + return decodeURIComponent(value); + } catch (error) { + return value; + } +}; + +/** The one store. It keeps no state of its own beyond what is in storage. */ +export const settingsStore = new SettingsStore(); diff --git a/assets/javascripts/templates/error_tmpl.js b/assets/javascripts/templates/error_tmpl.js index cbdf429ce7..4879301109 100644 --- a/assets/javascripts/templates/error_tmpl.js +++ b/assets/javascripts/templates/error_tmpl.js @@ -56,8 +56,8 @@ If you keep seeing this, you're likely behind a proxy or firewall that blocks cr * @returns {string} */ export const offlineError = function (reason, exception) { - if (reason === "cookie_blocked") { - return error(" Cookies must be enabled to use offline mode. "); + if (reason === "storage_blocked") { + return error(" Local storage must be enabled to use offline mode. "); } reason = (() => { diff --git a/assets/javascripts/templates/notif_tmpl.js b/assets/javascripts/templates/notif_tmpl.js index a433363933..210cf88c96 100644 --- a/assets/javascripts/templates/notif_tmpl.js +++ b/assets/javascripts/templates/notif_tmpl.js @@ -50,10 +50,10 @@ export const notifQuotaExceeded = () => " Unfortunately this quota can't be detected programmatically, and the database can't be opened while over the quota, so it had to be reset. ", ); -export const notifCookieBlocked = () => +export const notifStorageBlocked = () => textNotif( - " Please enable cookies. ", - " DevDocs will not work properly if cookies are disabled. ", + " Please enable local storage. ", + " DevDocs will not work properly if local storage is disabled. ", ); export const notifInvalidLocation = () => diff --git a/assets/javascripts/templates/pages/about_tmpl.js b/assets/javascripts/templates/pages/about_tmpl.js index a2d82ee9f6..27fc638ba1 100644 --- a/assets/javascripts/templates/pages/about_tmpl.js +++ b/assets/javascripts/templates/pages/about_tmpl.js @@ -91,7 +91,7 @@ export const aboutPage = function () {
  • We do not collect personal information through the app.
  • We use Google Analytics and Gauges to collect anonymous traffic information if you have given consent to this. You can change your decision in the settings.
  • We use Sentry to collect crash data and improve the app. -
  • The app uses cookies and local storage to store user preferences. +
  • The app uses local storage to store user preferences.
  • By using the app, you signify your acceptance of this policy. If you do not agree to this policy, please do not use the app.
  • If you have any questions regarding privacy, please email privacy@freecodecamp.org. \ diff --git a/assets/javascripts/tracking.js b/assets/javascripts/tracking.js index 4d28d48918..09bb7d218e 100644 --- a/assets/javascripts/tracking.js +++ b/assets/javascripts/tracking.js @@ -7,10 +7,11 @@ import { app } from "./app/app.js"; import { config } from "./app/config.js"; import { page, resetAnalytics } from "./lib/page.js"; +import { settingsStore } from "./lib/settings_store.js"; try { if (config.env === "production") { - if (Cookies.get("analyticsConsent") === "1") { + if (settingsStore.get("analyticsConsent") === 1) { (function (i, s, o, g, r, a, m) { i["GoogleAnalyticsObject"] = r; (i[r] = diff --git a/assets/javascripts/vendor.js b/assets/javascripts/vendor.js index ea7ddfe40c..ee5206d90d 100644 --- a/assets/javascripts/vendor.js +++ b/assets/javascripts/vendor.js @@ -1,6 +1,6 @@ // The third-party libraries, concatenated into one classic script. // -// They assign their globals (Cookies, Prism, Raven) rather than exporting, so +// They assign their globals (Prism, Raven) rather than exporting, so // they are loaded ahead of the module graph instead of being part of it. Being // one file also keeps them compressing against each other. diff --git a/assets/javascripts/vendor/cookies.js b/assets/javascripts/vendor/cookies.js deleted file mode 100644 index e592a5de8e..0000000000 --- a/assets/javascripts/vendor/cookies.js +++ /dev/null @@ -1,208 +0,0 @@ -/* - * Cookies.js - 1.2.3 (patched for SameSite=Strict and secure=true) - * https://github.com/ScottHamper/Cookies - * - * This is free and unencumbered software released into the public domain. - */ -(function (global, undefined) { - "use strict"; - - var factory = function (window) { - if (typeof window.document !== "object") { - throw new Error( - "Cookies.js requires a `window` with a `document` object", - ); - } - - var Cookies = function (key, value, options) { - return arguments.length === 1 - ? Cookies.get(key) - : Cookies.set(key, value, options); - }; - - // Allows for setter injection in unit tests - Cookies._document = window.document; - - // Used to ensure cookie keys do not collide with - // built-in `Object` properties - Cookies._cacheKeyPrefix = "cookey."; // Hurr hurr, :) - - Cookies._maxExpireDate = new Date("Fri, 31 Dec 9999 23:59:59 UTC"); - - Cookies.defaults = { - path: "/", - SameSite: "Strict", - secure: true, - }; - - Cookies.get = function (key) { - if (Cookies._cachedDocumentCookie !== Cookies._document.cookie) { - Cookies._renewCache(); - } - - var value = Cookies._cache[Cookies._cacheKeyPrefix + key]; - - return value === undefined ? undefined : decodeURIComponent(value); - }; - - Cookies.set = function (key, value, options) { - options = Cookies._getExtendedOptions(options); - options.expires = Cookies._getExpiresDate( - value === undefined ? -1 : options.expires, - ); - - Cookies._document.cookie = Cookies._generateCookieString( - key, - value, - options, - ); - - return Cookies; - }; - - Cookies.expire = function (key, options) { - return Cookies.set(key, undefined, options); - }; - - Cookies._getExtendedOptions = function (options) { - return { - path: (options && options.path) || Cookies.defaults.path, - domain: (options && options.domain) || Cookies.defaults.domain, - SameSite: (options && options.SameSite) || Cookies.defaults.SameSite, - expires: (options && options.expires) || Cookies.defaults.expires, - secure: - options && options.secure !== undefined - ? options.secure - : Cookies.defaults.secure, - }; - }; - - Cookies._isValidDate = function (date) { - return ( - Object.prototype.toString.call(date) === "[object Date]" && - !isNaN(date.getTime()) - ); - }; - - Cookies._getExpiresDate = function (expires, now) { - now = now || new Date(); - - if (typeof expires === "number") { - expires = - expires === Infinity - ? Cookies._maxExpireDate - : new Date(now.getTime() + expires * 1000); - } else if (typeof expires === "string") { - expires = new Date(expires); - } - - if (expires && !Cookies._isValidDate(expires)) { - throw new Error( - "`expires` parameter cannot be converted to a valid Date instance", - ); - } - - return expires; - }; - - Cookies._generateCookieString = function (key, value, options) { - key = key.replace(/[^#$&+\^`|]/g, encodeURIComponent); - key = key.replace(/\(/g, "%28").replace(/\)/g, "%29"); - value = (value + "").replace( - /[^!#$&-+\--:<-\[\]-~]/g, - encodeURIComponent, - ); - options = options || {}; - - var cookieString = key + "=" + value; - cookieString += options.path ? ";path=" + options.path : ""; - cookieString += options.domain ? ";domain=" + options.domain : ""; - cookieString += options.SameSite ? ";SameSite=" + options.SameSite : ""; - cookieString += options.expires - ? ";expires=" + options.expires.toUTCString() - : ""; - cookieString += options.secure ? ";secure" : ""; - - return cookieString; - }; - - Cookies._getCacheFromString = function (documentCookie) { - var cookieCache = {}; - var cookiesArray = documentCookie ? documentCookie.split("; ") : []; - - for (var i = 0; i < cookiesArray.length; i++) { - var cookieKvp = Cookies._getKeyValuePairFromCookieString( - cookiesArray[i], - ); - - if ( - cookieCache[Cookies._cacheKeyPrefix + cookieKvp.key] === undefined - ) { - cookieCache[Cookies._cacheKeyPrefix + cookieKvp.key] = - cookieKvp.value; - } - } - - return cookieCache; - }; - - Cookies._getKeyValuePairFromCookieString = function (cookieString) { - // "=" is a valid character in a cookie value according to RFC6265, so cannot `split('=')` - var separatorIndex = cookieString.indexOf("="); - - // IE omits the "=" when the cookie value is an empty string - separatorIndex = - separatorIndex < 0 ? cookieString.length : separatorIndex; - - var key = cookieString.substr(0, separatorIndex); - var decodedKey; - try { - decodedKey = decodeURIComponent(key); - } catch (e) { - if (console && typeof console.error === "function") { - console.error('Could not decode cookie with key "' + key + '"', e); - } - } - - return { - key: decodedKey, - value: cookieString.substr(separatorIndex + 1), // Defer decoding value until accessed - }; - }; - - Cookies._renewCache = function () { - Cookies._cache = Cookies._getCacheFromString(Cookies._document.cookie); - Cookies._cachedDocumentCookie = Cookies._document.cookie; - }; - - Cookies._areEnabled = function () { - var testKey = "cookies.js"; - var areEnabled = Cookies.set(testKey, 1).get(testKey) === "1"; - Cookies.expire(testKey); - return areEnabled; - }; - - Cookies.enabled = Cookies._areEnabled(); - - return Cookies; - }; - var cookiesExport = - global && typeof global.document === "object" ? factory(global) : factory; - - // AMD support - if (typeof define === "function" && define.amd) { - define(function () { - return cookiesExport; - }); - // CommonJS/Node.js support - } else if (typeof exports === "object") { - // Support Node.js specific `module.exports` (which can be a function) - if (typeof module === "object" && typeof module.exports === "object") { - exports = module.exports = cookiesExport; - } - // But always support CommonJS module 1.1.1 spec (`exports` cannot be a function) - exports.Cookies = cookiesExport; - } else { - global.Cookies = cookiesExport; - } -})(typeof window === "undefined" ? this : window); diff --git a/assets/javascripts/views/content/offline_page.js b/assets/javascripts/views/content/offline_page.js index d1f743ddba..3640eea622 100644 --- a/assets/javascripts/views/content/offline_page.js +++ b/assets/javascripts/views/content/offline_page.js @@ -28,8 +28,8 @@ export class OfflinePage extends View { /** Rebuilds the table from the docs and their install statuses. */ render() { - if (app.cookieBlocked) { - this.html(this.tmpl("offlineError", "cookie_blocked")); + if (app.storageBlocked) { + this.html(this.tmpl("offlineError", "storage_blocked")); return; } diff --git a/assets/javascripts/views/layout/document.js b/assets/javascripts/views/layout/document.js index f374f679cb..b2ae0f2a9d 100644 --- a/assets/javascripts/views/layout/document.js +++ b/assets/javascripts/views/layout/document.js @@ -153,10 +153,12 @@ export class AppDocument extends View { } break; case "accept-analytics": - Cookies.set("analyticsConsent", "1", { expires: 1e8 }) && app.reboot(); + app.settings.set("analyticsConsent", 1); + app.reboot(); break; case "decline-analytics": - Cookies.set("analyticsConsent", "0", { expires: 1e8 }) && app.reboot(); + app.settings.set("analyticsConsent", 0); + app.reboot(); break; } } diff --git a/assets/javascripts/views/layout/mobile.js b/assets/javascripts/views/layout/mobile.js index 7583ec9d0e..4071750870 100644 --- a/assets/javascripts/views/layout/mobile.js +++ b/assets/javascripts/views/layout/mobile.js @@ -31,8 +31,9 @@ export class Mobile extends View { * desktop-sized width. */ static detect() { - if (Cookies.get("override-mobile-detect") != null) { - return JSON.parse(Cookies.get("override-mobile-detect")); + const override = app.localStorage.get("override-mobile-detect"); + if (override != null) { + return !!override; } try { return ( diff --git a/test/app_test.rb b/test/app_test.rb index 4b5e4c3297..c2a97d71f2 100644 --- a/test/app_test.rb +++ b/test/app_test.rb @@ -68,39 +68,24 @@ def app end describe "/[doc]" do - it "renders when the doc exists and isn't enabled" do - set_cookie('docs=html~5') - get '/html~4/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER - assert last_response.ok? - end - - it "renders when the doc exists, is a default doc, and all docs are enabled" do - set_cookie('docs=') - get '/css/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER - assert last_response.ok? - end - - it "renders when the doc exists and is enabled" do - set_cookie('docs=html~5') + it "renders when the doc exists" do get '/html~5/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER assert last_response.ok? assert_nil last_response['Set-Cookie'] end - it "renders when the doc exists, has no version in the path, and isn't enabled" do - get '/html/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER + it "renders when the doc exists and has no versions" do + get '/css/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER assert last_response.ok? end - it "renders when the doc exists, has no version in the path, and a version is enabled" do - set_cookie('docs=html~5') + it "renders when the doc exists and has no version in the path" do get '/html/', {}, 'HTTP_USER_AGENT' => MODERN_BROWSER assert last_response.ok? assert_nil last_response['Set-Cookie'] end - it "renders when the doc exists and is enabled, and the request is from Googlebot" do - set_cookie('docs=html') + it "renders when the doc exists and the request is from Googlebot" do get '/html/', {}, 'HTTP_USER_AGENT' => 'Mozilla/5.0 (compatible; Googlebot/2.1; +https://www.google.com/bot.html)' assert last_response.ok? end diff --git a/test/assets/cookies_store_test.js b/test/assets/cookies_store_test.js deleted file mode 100644 index 5ea1a03607..0000000000 --- a/test/assets/cookies_store_test.js +++ /dev/null @@ -1,191 +0,0 @@ -// @ts-check - -import assert from "node:assert/strict"; -import test from "node:test"; - -import { CookiesStore } from "../../assets/javascripts/lib/cookies_store.js"; - -// A cookie jar the vendored Cookies.js can write to: keys and values stay -// percent-encoded, as they are on a real `document.cookie`, and a write whose -// expiry has passed deletes the key. -/** @type {Map} */ -const jar = new Map(); -let cookiesAccepted = true; - -Object.defineProperty(document, "cookie", { - get: () => - [...jar].map(([key, value]) => `${key}=${value}`).join("; "), - set: (string) => { - if (!cookiesAccepted) return; - const [pair, ...attributes] = string.split(";"); - const separator = pair.indexOf("="); - const key = pair.slice(0, separator); - const expires = attributes - .map((attribute) => attribute.trim()) - .find((attribute) => /^expires=/i.test(attribute)); - - if (expires && new Date(expires.slice(8)) <= new Date()) { - jar.delete(key); - } else { - jar.set(key, pair.slice(separator + 1)); - } - }, - configurable: true, -}); - -/** @type {Map} */ -const storage = new Map(); - -Object.defineProperty(globalThis, "localStorage", { - value: { - getItem: (/** @type {string} */ key) => - storage.has(key) ? storage.get(key) : null, - setItem: (/** @type {string} */ key, /** @type {string} */ value) => - storage.set(key, String(value)), - removeItem: (/** @type {string} */ key) => storage.delete(key), - clear: () => storage.clear(), - }, - writable: true, - configurable: true, -}); - -// The store reads the vendored library off the global, and the library binds -// itself to whatever `window` it is loaded with, so hand it the document above -// before it evaluates. -// @ts-ignore -- the fake window from setup.js has no document until now. -window.document = document; -await import("../../assets/javascripts/vendor/cookies.js"); -Object.defineProperty(globalThis, "Cookies", { - // @ts-ignore -- where the library puts itself under Node. - value: window.Cookies, - writable: true, - configurable: true, -}); - -/** Wipes both stores, as a fresh browser profile would. */ -const reset = () => { - jar.clear(); - storage.clear(); - cookiesAccepted = true; -}; - -/** Drops the cookies while leaving localStorage alone, as Safari's seven-day cap does. */ -const expireCookies = () => jar.clear(); - -test("stores and reads back values, parsing the integers", () => { - reset(); - const store = new CookiesStore(); - - store.set("docs", "css/javascript"); - store.set("size", 320); - store.set("hideIntro", true); - - assert.equal(store.get("docs"), "css/javascript"); - assert.equal(store.get("size"), 320); - assert.equal(store.get("hideIntro"), 1); - assert.equal(store.get("missing"), undefined); -}); - -test("puts back the cookies the browser has dropped", () => { - reset(); - new CookiesStore().set("docs", "css/javascript"); - - expireCookies(); - const store = new CookiesStore(); - - assert.equal(store.get("docs"), "css/javascript"); - assert.match(document.cookie, /docs=css\/javascript/); -}); - -test("puts back the settings of a visitor who has no mirror yet", () => { - reset(); - // The cookie is there, but predates the mirror, as on the visit that first - // runs this version of the app. - document.cookie = "docs=css/javascript"; - new CookiesStore(); - - expireCookies(); - assert.equal(new CookiesStore().get("docs"), "css/javascript"); -}); - -test("keeps a value intact when it is restored, rather than re-escaping it", () => { - reset(); - new CookiesStore().set("layout", "_max-width _sidebar-hidden"); - - expireCookies(); - const store = new CookiesStore(); - - assert.equal(store.get("layout"), "_max-width _sidebar-hidden"); - assert.deepEqual(store.dump(), { layout: "_max-width _sidebar-hidden" }); -}); - -test("does not put back a deleted value", () => { - reset(); - const store = new CookiesStore(); - store.set("docs", "css/javascript"); - store.del("docs"); - - assert.equal(store.get("docs"), undefined); - - expireCookies(); - assert.equal(new CookiesStore().get("docs"), undefined); -}); - -test("does not put anything back after a reset", () => { - reset(); - const store = new CookiesStore(); - store.set("docs", "css/javascript"); - store.set("size", 320); - store.reset(); - - assert.equal(store.get("docs"), undefined); - assert.deepEqual(new CookiesStore().dump(), {}); -}); - -test("falls back to the mirror, and still reports the block, when cookies are refused", () => { - reset(); - const store = new CookiesStore(); - /** @type {unknown[]} */ - const blocked = []; - const onBlocked = CookiesStore.onBlocked; - CookiesStore.onBlocked = (key, value, actual) => - blocked.push([key, value, actual]); - - try { - cookiesAccepted = false; - store.set("docs", "css/javascript"); - - assert.deepEqual(blocked, [["docs", "css/javascript", undefined]]); - assert.equal(store.get("docs"), "css/javascript"); - assert.equal(new CookiesStore().get("docs"), "css/javascript"); - } finally { - CookiesStore.onBlocked = onBlocked; - } -}); - -test("keeps a setting another tab has written since the mirror was read", () => { - reset(); - const tabA = new CookiesStore(); - const tabB = new CookiesStore(); - - tabA.set("theme", "dark"); - tabB.set("size", 320); - - expireCookies(); - const store = new CookiesStore(); - - assert.equal(store.get("theme"), "dark"); - assert.equal(store.get("size"), 320); -}); - -test("keeps reading a value whose cookie the browser has refused", () => { - reset(); - const store = new CookiesStore(); - store.set("docs", "css"); - - cookiesAccepted = false; - store.set("docs", "css/javascript"); - - assert.equal(store.get("docs"), "css/javascript"); - assert.deepEqual(store.dump(), { docs: "css/javascript" }); -}); diff --git a/test/assets/settings_store_test.js b/test/assets/settings_store_test.js new file mode 100644 index 0000000000..70258ecd23 --- /dev/null +++ b/test/assets/settings_store_test.js @@ -0,0 +1,166 @@ +// @ts-check + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { SettingsStore } from "../../assets/javascripts/lib/settings_store.js"; + +/** @type {Map} */ +const storage = new Map(); +let storageWritable = true; + +Object.defineProperty(globalThis, "localStorage", { + value: { + getItem: (/** @type {string} */ key) => + storage.has(key) ? storage.get(key) : null, + setItem: (/** @type {string} */ key, /** @type {string} */ value) => { + if (!storageWritable) throw new Error("storage is full"); + storage.set(key, String(value)); + }, + removeItem: (/** @type {string} */ key) => storage.delete(key), + clear: () => storage.clear(), + }, + writable: true, + configurable: true, +}); + +// A cookie jar the migration can read and expire: keys and values stay +// percent-encoded, as they are on a real `document.cookie`. +/** @type {Map} */ +const jar = new Map(); + +Object.defineProperty(document, "cookie", { + get: () => [...jar].map(([key, value]) => `${key}=${value}`).join("; "), + set: (/** @type {string} */ string) => { + const [pair, ...attributes] = string.split(";"); + const separator = pair.indexOf("="); + const key = pair.slice(0, separator); + const expires = attributes + .map((attribute) => attribute.trim()) + .find((attribute) => /^expires=/i.test(attribute)); + + if (expires && new Date(expires.slice(8)) <= new Date()) { + jar.delete(key); + } else { + jar.set(key, pair.slice(separator + 1)); + } + }, + configurable: true, +}); + +/** Wipes both stores, as a fresh browser profile would. */ +const reset = () => { + jar.clear(); + storage.clear(); + storageWritable = true; +}; + +test("stores and reads back values, parsing the integers", () => { + reset(); + const store = new SettingsStore(); + + store.set("docs", "css/javascript"); + store.set("size", 320); + store.set("hideIntro", true); + + assert.equal(store.get("docs"), "css/javascript"); + assert.equal(store.get("size"), 320); + assert.equal(store.get("hideIntro"), 1); + assert.equal(store.get("missing"), undefined); +}); + +test("deletes a key written as false, and clears everything on reset", () => { + reset(); + const store = new SettingsStore(); + store.set("docs", "css"); + store.set("hideIntro", true); + + store.set("hideIntro", false); + assert.deepEqual(store.dump(), { docs: "css" }); + + store.del("docs"); + assert.deepEqual(store.dump(), {}); + + store.set("docs", "css"); + store.reset(); + assert.deepEqual(new SettingsStore().dump(), {}); +}); + +test("sees what another store has written", () => { + reset(); + const store = new SettingsStore(); + new SettingsStore().set("theme", "dark"); + + assert.equal(store.get("theme"), "dark"); +}); + +test("takes in the settings left in cookies, and expires them", () => { + reset(); + document.cookie = "docs=css/javascript"; + document.cookie = "size=320"; + + const store = new SettingsStore(); + + assert.equal(store.get("docs"), "css/javascript"); + assert.equal(store.get("size"), 320); + assert.equal(document.cookie, "", "the cookies should be gone"); +}); + +test("decodes a cookie value rather than storing its escapes", () => { + reset(); + document.cookie = "layout=_max-width%20_sidebar-hidden"; + + assert.equal( + new SettingsStore().get("layout"), + "_max-width _sidebar-hidden", + ); +}); + +test("keeps the stored value when a cookie of the same name is left over", () => { + reset(); + new SettingsStore().set("theme", "dark"); + document.cookie = "theme=default"; + + assert.equal(new SettingsStore().get("theme"), "dark"); + assert.equal(document.cookie, ""); +}); + +test("leaves the analytics vendors' own cookies alone", () => { + reset(); + document.cookie = "_ga=GA1.2.3"; + + const store = new SettingsStore(); + + assert.deepEqual(store.dump(), {}); + assert.equal(document.cookie, "_ga=GA1.2.3"); +}); + +test("drops the consent-asked cookie rather than making it permanent", () => { + reset(); + document.cookie = "analyticsConsentAsked=1"; + document.cookie = "analyticsConsent=1"; + + const store = new SettingsStore(); + + assert.deepEqual(store.dump(), { analyticsConsent: "1" }); + assert.equal(document.cookie, ""); +}); + +test("reports a write that doesn't stick", () => { + reset(); + const store = new SettingsStore(); + /** @type {unknown[]} */ + const blocked = []; + const onBlocked = SettingsStore.onBlocked; + SettingsStore.onBlocked = (key, value, actual) => + blocked.push([key, value, actual]); + + try { + storageWritable = false; + store.set("docs", "css/javascript"); + + assert.deepEqual(blocked, [["docs", "css/javascript", undefined]]); + } finally { + SettingsStore.onBlocked = onBlocked; + } +}); diff --git a/test/assets/setup.js b/test/assets/setup.js index c338472271..91543ce84a 100644 --- a/test/assets/setup.js +++ b/test/assets/setup.js @@ -59,6 +59,7 @@ const define = (name, value) => define("document", { ...element, + cookie: "", documentElement: element, body: element, createElement: () => ({ ...element }), From 67a48b337d2924be6735406f870a8b786615c622 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 16:31:51 +0200 Subject: [PATCH 04/12] Cache the doc indexes in IndexedDB instead of localStorage 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. --- assets/javascripts/app/app.js | 1 + assets/javascripts/app/db.js | 183 +++++++++++++++++- assets/javascripts/app/offline_backup.js | 37 ++-- assets/javascripts/lib/local_storage_store.js | 12 ++ assets/javascripts/models/doc.js | 74 +++---- .../templates/pages/offline_tmpl.js | 2 +- test/assets/doc_cache_test.js | 180 +++++++++++++++++ 7 files changed, 424 insertions(+), 65 deletions(-) create mode 100644 test/assets/doc_cache_test.js diff --git a/assets/javascripts/app/app.js b/assets/javascripts/app/app.js index 2c52255026..bd0e4aa9ee 100644 --- a/assets/javascripts/app/app.js +++ b/assets/javascripts/app/app.js @@ -190,6 +190,7 @@ export class App extends Events { delete this.DOCS; this.migrateDocs(); await this.migrateToLatestVersions(); + this.db.migrateIndexes(); this.docs.load(this.start.bind(this), this.onBootError.bind(this), { readCache: true, writeCache: true, diff --git a/assets/javascripts/app/db.js b/assets/javascripts/app/db.js index 6c8aaf50a8..71d9f7231c 100644 --- a/assets/javascripts/app/db.js +++ b/assets/javascripts/app/db.js @@ -2,6 +2,7 @@ import { app } from "./app.js"; import { ajax } from "../lib/ajax.js"; +import { SettingsStore } from "../lib/settings_store.js"; import { $ } from "../lib/util.js"; /** @import { Doc } from "../models/doc.js" */ /** @import { Entry } from "../models/entry.js" */ @@ -51,6 +52,18 @@ export class DB { static NAME = "docs"; static VERSION = 15; + /** + * The docs' entry indexes, by slug, as `[mtime, index]`. Not one store per + * doc: a doc's index is cached before it is enabled (see App#enableDoc), so + * a store of its own wouldn't exist yet. And not inside the doc's own store + * either, where `index` is the doc's home page and DB#store clears + * everything it holds on install. + */ + static INDEXES_STORE = "indexes"; + + /** The localStorage keys that aren't an index an older app left behind. */ + static NOT_INDEXES = [SettingsStore.KEY, "override-mobile-detect"]; + /** Probes for IndexedDB support and prepares the callback queue. */ constructor() { this.versionMultipler = $.isIE() ? 1e5 : 1e9; @@ -240,7 +253,8 @@ export class DB { } /** - * Creates an object store per enabled doc. + * Creates an object store per enabled doc, plus the two the app keeps for + * itself: the installed docs' mtimes, and the cached entry indexes. * * @param {IDBVersionChangeEvent} event */ @@ -253,10 +267,12 @@ export class DB { const objectStoreNames = $.makeArray(db.objectStoreNames); - if (!$.arrayDelete(objectStoreNames, "docs")) { - try { - db.createObjectStore("docs"); - } catch (error) {} + for (var store of ["docs", DB.INDEXES_STORE]) { + if (!$.arrayDelete(objectStoreNames, store)) { + try { + db.createObjectStore(store); + } catch (error) {} + } } for (var doc of app.docs.all()) { @@ -482,6 +498,155 @@ export class DB { }); } + /** + * Reads a doc's cached entry index. + * + * @param {Doc} doc + * @param {number} mtime The build to read it for; one cached for an earlier + * build is dropped rather than returned. + * @param {(index?: unknown) => void} fn Called with the index, or with + * nothing when there isn't a usable one. + */ + loadIndex(doc, mtime, fn) { + this.db((db) => { + let req; + try { + req = this.indexesStore(db, "readonly").get(doc.slug); + } catch (error) { + this.onMissingIndexesStore(error); + fn(); + return; + } + + req.onsuccess = () => { + const cached = req.result; + if (!cached) { + fn(); + } else if (cached[0] === mtime) { + fn(cached[1]); + } else { + this.deleteIndex(doc); + fn(); + } + }; + req.onerror = function (event) { + event.preventDefault(); + fn(); + }; + }); + } + + /** + * @param {Doc} doc + * @param {number} mtime The build the index was fetched for. + * @param {unknown} index + * @param {boolean} [_retry] Internal: whether a failure may bump the schema and try again. + */ + storeIndex(doc, mtime, index, _retry) { + if (_retry == null) { + _retry = true; + } + this.db((db) => { + try { + this.indexesStore(db, "readwrite").put([mtime, index], doc.slug); + } catch (error) { + // The store is missing for anyone whose database predates it. Bumping + // the schema creates it (see onUpgradeNeeded). + if (this.onMissingIndexesStore(error) && _retry) { + setTimeout(() => this.storeIndex(doc, mtime, index, false), 0); + } + } + }); + } + + /** @param {Doc} doc */ + deleteIndex(doc) { + this.db((db) => { + try { + this.indexesStore(db, "readwrite").delete(doc.slug); + } catch (error) { + this.onMissingIndexesStore(error); + } + }); + } + + /** + * Takes in the indexes an earlier version of the app cached in localStorage, + * and clears out everything it left there. Called from the boot, once the + * enabled docs are known — opening the database before that would upgrade it + * into having no doc stores at all. + * + * An index is dropped from localStorage whether or not it makes it into the + * database: it is a cache, and the doc falls back to the network. + * + * @param {boolean} [_retry] Internal: whether a failure may bump the schema + * and try again. + */ + migrateIndexes(_retry) { + if (_retry == null) { + _retry = true; + } + + const keys = app.localStorage + .keys() + .filter((key) => !DB.NOT_INDEXES.includes(key)); + + if (keys.length === 0) { + return; + } + + this.db((db) => { + let store; + try { + store = this.indexesStore(db, "readwrite"); + } catch (error) { + // Wait for the store rather than sweeping without one, which would + // drop every index instead of moving it. + if (this.onMissingIndexesStore(error) && _retry) { + setTimeout(() => this.migrateIndexes(false), 0); + return; + } + } + + // One at a time, rather than reading every index into memory first. + for (var key of keys) { + const cached = app.localStorage.get(key); + if (store && isCachedIndex(cached)) { + try { + store.put(cached, key); + } catch (error) {} + } + app.localStorage.del(key); + } + }); + } + + /** + * @param {IDBDatabase | undefined} db + * @param {IDBTransactionMode} mode + * @returns {IDBObjectStore} Throws when the database is missing, or hasn't + * got the store yet, which every caller treats as a cache miss. + */ + indexesStore(db, mode) { + return this.idbTransaction(db, { + stores: [DB.INDEXES_STORE], + mode, + }).objectStore(DB.INDEXES_STORE); + } + + /** + * @param {{ name?: string }} error + * @returns {boolean} Whether the store was the thing that was missing, in + * which case the schema has been bumped so that the next open creates it. + */ + onMissingIndexesStore(error) { + if (error?.name !== "NotFoundError") { + return false; + } + this.migrate(); + return true; + } + /** * @param {Doc} doc * @returns {number | false | undefined} `undefined` when the cache isn't loaded yet. @@ -806,3 +971,11 @@ export class DB { return app.settings.get("schema"); } } + +/** + * @param {unknown} value + * @returns {boolean} Whether `value` is an index as the old localStorage cache + * stored it: the mtime it was fetched at, and the index itself. + */ +const isCachedIndex = (value) => + Array.isArray(value) && value.length === 2 && typeof value[0] === "number"; diff --git a/assets/javascripts/app/offline_backup.js b/assets/javascripts/app/offline_backup.js index 7eae968d35..27b7c91895 100644 --- a/assets/javascripts/app/offline_backup.js +++ b/assets/javascripts/app/offline_backup.js @@ -25,8 +25,8 @@ import { app } from "./app.js"; */ /** - * Exports the offline data (the pages stored in IndexedDB and the index files - * cached in localStorage) to a JSON file, and imports it back — either to + * Exports the offline data (the pages and the index files, both in IndexedDB) + * to a JSON file, and imports it back — either to * restore a backup after the browser evicted the data, or to move the * documentations to another computer without downloading them again. */ @@ -78,15 +78,24 @@ export class OfflineBackup { onProgress(doc, i, docs.length); app.db.dump(doc, (result) => { - if (result) { + if (!result) { + setTimeout(next, 0); + return; + } + + // Ship the index file too, so that the doc can be used on a computer + // that never downloaded it (the app falls back to the network + // otherwise). Asking for it by the stored mtime leaves behind one that + // belongs to a different build. + app.db.loadIndex(doc, result.mtime, (index) => { // Serialize each doc on its own instead of building one big object, // to avoid holding the whole backup in memory twice. chunks.push( (count++ === 0 ? "" : ",") + - JSON.stringify(this.serializeDoc(doc, result)), + JSON.stringify(this.serializeDoc(doc, result, index)), ); - } - setTimeout(next, 0); + setTimeout(next, 0); + }); }); }; @@ -96,15 +105,13 @@ export class OfflineBackup { /** * @param {Doc} doc * @param {{ mtime: number, data: unknown }} result The doc's stored database. + * @param {unknown} [index] The doc's entry index, when one was cached. * @returns {unknown} One entry of the backup's `docs` array. */ - serializeDoc(doc, result) { + serializeDoc(doc, result, index) { const entry = { slug: doc.slug, mtime: result.mtime, db: result.data }; - const index = app.localStorage.get(doc.slug); - // Ship the index file too, so that the doc can be used on a computer that - // never downloaded it (the app falls back to the network otherwise). - if (index && index[0] === result.mtime) { - entry.index = index[1]; + if (index !== undefined) { + entry.index = index; } return entry; } @@ -197,9 +204,9 @@ export class OfflineBackup { mtime, () => { if (this.isValidIndex(entry.index)) { - // Keyed by the backup's mtime so that Doc#_getCache discards it - // when the doc has been updated since the backup was made. - app.localStorage.set(doc.slug, [mtime, entry.index]); + // Keyed by the backup's mtime so that the store discards it when + // the doc has been updated since the backup was made. + app.db.storeIndex(doc, mtime, entry.index); } imported.push(doc); setTimeout(next, 0); diff --git a/assets/javascripts/lib/local_storage_store.js b/assets/javascripts/lib/local_storage_store.js index ee28bc16bf..95448ab2c3 100644 --- a/assets/javascripts/lib/local_storage_store.js +++ b/assets/javascripts/lib/local_storage_store.js @@ -8,6 +8,7 @@ * @property {(key: string) => unknown} get * @property {(key: string, value: unknown) => boolean | undefined} set * @property {(key: string) => boolean | undefined} del + * @property {() => string[]} keys * @property {() => boolean | undefined} reset */ @@ -52,6 +53,17 @@ export const LocalStorageStore = class LocalStorageStore { } catch (error) {} } + /** + * @returns {string[]} Every key, or an empty list when storage is unreadable. + */ + keys() { + try { + return Object.keys(localStorage); + } catch (error) { + return []; + } + } + /** * @returns {boolean | undefined} `true` when cleared, `undefined` when it failed. */ diff --git a/assets/javascripts/models/doc.js b/assets/javascripts/models/doc.js index f8c3bb173c..ff1264121b 100644 --- a/assets/javascripts/models/doc.js +++ b/assets/javascripts/models/doc.js @@ -160,67 +160,53 @@ export class Doc extends Model { if (options == null) { options = {}; } - if (options.readCache && this._loadFromCache(onSuccess)) { + + const fromNetwork = () => { + ajax({ + url: this.indexUrl(), + success: (data) => { + this.reset(data); + onSuccess(); + if (options.writeCache) { + this._setCache(data); + } + }, + error: onError, + }); + }; + + if (!options.readCache) { + fromNetwork(); return; } - const callback = (data) => { - this.reset(data); - onSuccess(); - if (options.writeCache) { - this._setCache(data); + this._getCache((data) => { + if (data) { + this.reset(data); + onSuccess(); + } else { + fromNetwork(); } - }; - - return ajax({ - url: this.indexUrl(), - success: callback, - error: onError, }); } /** Drops the cached index. */ clearCache() { - app.localStorage.del(this.slug); + app.db.deleteIndex(this); } /** - * @param {() => void} onSuccess Called asynchronously, to match the network path. - * @returns {boolean | undefined} `true` when the cache was used. + * @param {(index?: unknown) => void} fn Called with the cached index, or with + * nothing when it is missing or stale. A hit is always asynchronous, and a + * miss leads to the network, so `load` never calls back synchronously. */ - _loadFromCache(onSuccess) { - const data = this._getCache(); - if (!data) { - return; - } - - const callback = () => { - this.reset(data); - onSuccess(); - }; - - setTimeout(callback, 0); - return true; - } - - /** @returns {unknown} The cached index, or `undefined` when it is missing or stale. */ - _getCache() { - const data = app.localStorage.get(this.slug); - if (!data) { - return; - } - - if (data[0] === this.mtime) { - return data[1]; - } else { - this.clearCache(); - return; - } + _getCache(fn) { + app.db.loadIndex(this, this.mtime, fn); } /** @param {unknown} data */ _setCache(data) { - app.localStorage.set(this.slug, [this.mtime, data]); + app.db.storeIndex(this, this.mtime, data); } /** diff --git a/assets/javascripts/templates/pages/offline_tmpl.js b/assets/javascripts/templates/pages/offline_tmpl.js index cf784aa211..98312f84ec 100644 --- a/assets/javascripts/templates/pages/offline_tmpl.js +++ b/assets/javascripts/templates/pages/offline_tmpl.js @@ -46,7 +46,7 @@ export const offlinePage = (docs, hasPersistence, isPersistent) => `\
    How does this work?
    Each page is cached as a key-value pair in IndexedDB (downloaded from a single file).
    - The app also uses Service Workers and localStorage to cache the assets and index files. + The index files are cached in IndexedDB as well, and the app itself by a Service Worker.
    Can I close the tab/browser?
    ${canICloseTheTab()}
    How do I move the documentations to another computer? diff --git a/test/assets/doc_cache_test.js b/test/assets/doc_cache_test.js new file mode 100644 index 0000000000..76fe065c98 --- /dev/null +++ b/test/assets/doc_cache_test.js @@ -0,0 +1,180 @@ +// @ts-check + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { app } from "../../assets/javascripts/app/app.js"; +import { LocalStorageStore } from "../../assets/javascripts/lib/local_storage_store.js"; +import { DB } from "../../assets/javascripts/app/db.js"; +import { Doc } from "../../assets/javascripts/models/doc.js"; + +// ajax() goes through XMLHttpRequest, so a fake one is enough to see whether a +// load reached the network, and to answer it. +/** @type {any[]} */ +const requests = []; + +Object.defineProperty(globalThis, "XMLHttpRequest", { + value: class { + open(/** @type {string} */ type, /** @type {string} */ url) { + this.url = url; + requests.push(this); + } + setRequestHeader() {} + send() {} + abort() {} + /** Answers the request the way ajax() reads a response back. */ + respond(/** @type {unknown} */ index) { + this.readyState = 4; + this.status = 200; + this.responseText = JSON.stringify(index); + /** @type {any} */ (this).onreadystatechange(); + } + }, + writable: true, + configurable: true, +}); + +/** The index a doc loads, kept minimal: the shape is all Doc#reset touches. */ +const INDEX = { entries: [], types: [] }; + +/** Stands in for the database, with one doc's index cached in it. */ +const stubStore = (cached) => { + /** @type {{ reads: string[], written: unknown[] }} */ + const calls = { reads: [], written: [] }; + app.db = /** @type {any} */ ({ + loadIndex(doc, mtime, fn) { + calls.reads.push(`${doc.slug}@${mtime}`); + // Asynchronous, as the real one is. + setTimeout(() => fn(cached), 0); + }, + storeIndex(doc, mtime, index) { + calls.written.push([doc.slug, mtime, index]); + }, + deleteIndex(doc) { + calls.written.push([doc.slug, null]); + }, + }); + return calls; +}; + +const newDoc = () => new Doc({ name: "CSS", slug: "css", mtime: 42 }); + +test("a cached index is used instead of the network", async () => { + requests.length = 0; + const calls = stubStore(INDEX); + const doc = newDoc(); + + await new Promise((resolve) => + doc.load(() => resolve(undefined), () => assert.fail("errored"), { + readCache: true, + }), + ); + + assert.deepEqual(calls.reads, ["css@42"], "the store should be asked once"); + assert.equal(requests.length, 0, "nothing should have been fetched"); + assert.ok(doc.entries, "the doc should have been reset from the cache"); +}); + +test("a miss falls through to the network, and stores what it fetched", async () => { + requests.length = 0; + const calls = stubStore(undefined); + const doc = newDoc(); + + const loaded = new Promise((resolve) => + doc.load(() => resolve(undefined), () => assert.fail("errored"), { + readCache: true, + writeCache: true, + }), + ); + + // The store answers on a timer, so the request is only made after it has. + await new Promise((resolve) => setTimeout(resolve, 0)); + assert.equal(requests.length, 1, "the index should have been fetched"); + assert.match(requests[0].url, /\/css\/index\.json\?42$/); + + requests[0].respond(INDEX); + await loaded; + + assert.deepEqual(calls.written, [["css", 42, INDEX]]); +}); + +test("the cache is left alone when the caller doesn't ask for it", async () => { + requests.length = 0; + const calls = stubStore(INDEX); + const doc = newDoc(); + + const loaded = new Promise((resolve) => + doc.load(() => resolve(undefined), () => assert.fail("errored")), + ); + + assert.deepEqual(calls.reads, [], "the store shouldn't have been read"); + assert.equal(requests.length, 1); + + requests[0].respond(INDEX); + await loaded; + + assert.deepEqual(calls.written, [], "and nothing should have been written"); +}); + +test("a doc drops its cached index when it is cleared", () => { + const calls = stubStore(INDEX); + newDoc().clearCache(); + assert.deepEqual(calls.written, [["css", null]]); +}); + +// Items are own properties of a real Storage, and its methods live on the +// prototype, which is what makes Object.keys() return the keys and nothing +// else. +class FakeStorage { + constructor(/** @type {Record} */ entries) { + Object.assign(this, entries); + } + getItem(/** @type {string} */ key) { + return Object.prototype.hasOwnProperty.call(this, key) ? this[key] : null; + } + setItem(/** @type {string} */ key, /** @type {string} */ value) { + this[key] = String(value); + } + removeItem(/** @type {string} */ key) { + delete this[key]; + } + clear() { + for (const key of Object.keys(this)) delete this[key]; + } +} + +test("the indexes an older app cached in localStorage are taken in", () => { + const stored = new FakeStorage({ + settings: '{"docs":"css"}', + "override-mobile-detect": "true", + css: '[42,{"entries":[]}]', + html: '[7,{"entries":[]}]', + junk: '"not an index"', + }); + + Object.defineProperty(globalThis, "localStorage", { + value: stored, + writable: true, + configurable: true, + }); + + app.localStorage = new LocalStorageStore(); + const db = new DB(); + /** @type {unknown[]} */ + const puts = []; + db.db = (fn) => fn(/** @type {any} */ ({})); + db.indexesStore = () => + /** @type {any} */ ({ put: (value, key) => puts.push([key, value]) }); + + db.migrateIndexes(); + + assert.deepEqual(puts, [ + ["css", [42, { entries: [] }]], + ["html", [7, { entries: [] }]], + ]); + assert.deepEqual( + Object.keys(stored), + ["settings", "override-mobile-detect"], + "everything else should have been cleared out", + ); +}); From 9995eb2a899cc47848f712634b9df57b4c63789f Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 16:59:43 +0200 Subject: [PATCH 05/12] Trim the settings store and the index cache 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. --- assets/javascripts/app/app.js | 4 +- assets/javascripts/app/db.js | 173 ++++++------------ assets/javascripts/collections/docs.js | 7 - assets/javascripts/lib/local_storage_store.js | 12 -- assets/javascripts/lib/page.js | 4 +- assets/javascripts/lib/settings_store.js | 83 +++++---- assets/javascripts/models/doc.js | 31 +--- test/assets/doc_cache_test.js | 89 +++------ test/assets/settings_store_test.js | 65 ++----- views/service-worker.js.erb | 5 +- 10 files changed, 155 insertions(+), 318 deletions(-) diff --git a/assets/javascripts/app/app.js b/assets/javascripts/app/app.js index bd0e4aa9ee..9af2b7c5fe 100644 --- a/assets/javascripts/app/app.js +++ b/assets/javascripts/app/app.js @@ -190,7 +190,6 @@ export class App extends Events { delete this.DOCS; this.migrateDocs(); await this.migrateToLatestVersions(); - this.db.migrateIndexes(); this.docs.load(this.start.bind(this), this.onBootError.bind(this), { readCache: true, writeCache: true, @@ -415,8 +414,7 @@ export class App extends Events { /** Drops the cached indexes and reloads the app. */ reload() { - this.docs.clearCache(); - this.disabledDocs.clearCache(); + this.db.clearIndexes(); if (this.serviceWorker) { this.serviceWorker.reload(); } else { diff --git a/assets/javascripts/app/db.js b/assets/javascripts/app/db.js index 71d9f7231c..d47298623d 100644 --- a/assets/javascripts/app/db.js +++ b/assets/javascripts/app/db.js @@ -2,7 +2,6 @@ import { app } from "./app.js"; import { ajax } from "../lib/ajax.js"; -import { SettingsStore } from "../lib/settings_store.js"; import { $ } from "../lib/util.js"; /** @import { Doc } from "../models/doc.js" */ /** @import { Entry } from "../models/entry.js" */ @@ -61,9 +60,6 @@ export class DB { */ static INDEXES_STORE = "indexes"; - /** The localStorage keys that aren't an index an older app left behind. */ - static NOT_INDEXES = [SettingsStore.KEY, "override-mobile-detect"]; - /** Probes for IndexedDB support and prepares the callback queue. */ constructor() { this.versionMultipler = $.isIE() ? 1e5 : 1e9; @@ -498,36 +494,53 @@ export class DB { }); } + /** + * Runs `fn` with the indexes store, or with nothing when the database can't + * hand it over — unavailable, or old enough to predate the store, in which + * case the schema is bumped so that the next open creates it. + * + * @param {IDBTransactionMode} mode + * @param {(store?: IDBObjectStore) => void} fn + */ + indexes(mode, fn) { + this.db((db) => { + let store; + if (db) { + try { + store = this.idbTransaction(db, { + stores: [DB.INDEXES_STORE], + mode, + }).objectStore(DB.INDEXES_STORE); + } catch (error) { + if (error?.name === "NotFoundError") { + this.migrate(); + } + } + } + fn(store); + }); + } + /** * Reads a doc's cached entry index. * * @param {Doc} doc * @param {number} mtime The build to read it for; one cached for an earlier - * build is dropped rather than returned. + * build is passed over, and overwritten when the doc is fetched again. * @param {(index?: unknown) => void} fn Called with the index, or with * nothing when there isn't a usable one. */ loadIndex(doc, mtime, fn) { - this.db((db) => { - let req; - try { - req = this.indexesStore(db, "readonly").get(doc.slug); - } catch (error) { - this.onMissingIndexesStore(error); - fn(); + this.indexes("readonly", (store) => { + const req = store?.get(doc.slug); + if (!req) { + fn(this.importIndex(doc, mtime)); return; } req.onsuccess = () => { const cached = req.result; - if (!cached) { - fn(); - } else if (cached[0] === mtime) { - fn(cached[1]); - } else { - this.deleteIndex(doc); - fn(); - } + fn(cached?.[0] === mtime ? cached[1] : this.importIndex(doc, mtime)); }; req.onerror = function (event) { event.preventDefault(); @@ -540,111 +553,45 @@ export class DB { * @param {Doc} doc * @param {number} mtime The build the index was fetched for. * @param {unknown} index - * @param {boolean} [_retry] Internal: whether a failure may bump the schema and try again. */ - storeIndex(doc, mtime, index, _retry) { - if (_retry == null) { - _retry = true; - } - this.db((db) => { - try { - this.indexesStore(db, "readwrite").put([mtime, index], doc.slug); - } catch (error) { - // The store is missing for anyone whose database predates it. Bumping - // the schema creates it (see onUpgradeNeeded). - if (this.onMissingIndexesStore(error) && _retry) { - setTimeout(() => this.storeIndex(doc, mtime, index, false), 0); - } - } - }); + storeIndex(doc, mtime, index) { + this.indexes("readwrite", (store) => store?.put([mtime, index], doc.slug)); } /** @param {Doc} doc */ deleteIndex(doc) { - this.db((db) => { - try { - this.indexesStore(db, "readwrite").delete(doc.slug); - } catch (error) { - this.onMissingIndexesStore(error); - } - }); + this.indexes("readwrite", (store) => store?.delete(doc.slug)); + } + + /** Drops every cached index. */ + clearIndexes() { + this.indexes("readwrite", (store) => store?.clear()); } /** - * Takes in the indexes an earlier version of the app cached in localStorage, - * and clears out everything it left there. Called from the boot, once the - * enabled docs are known — opening the database before that would upgrade it - * into having no doc stores at all. + * Moves an index an earlier version of the app cached in localStorage into + * the database, on the read that goes looking for it — nothing is moved for + * a doc that is never loaded, and nothing holds up the boot. * - * An index is dropped from localStorage whether or not it makes it into the - * database: it is a cache, and the doc falls back to the network. + * Remove once the app has had a release or two to empty localStorage out. * - * @param {boolean} [_retry] Internal: whether a failure may bump the schema - * and try again. + * @param {Doc} doc + * @param {number} mtime + * @returns {unknown} The index, when localStorage held a current one. */ - migrateIndexes(_retry) { - if (_retry == null) { - _retry = true; + importIndex(doc, mtime) { + const cached = app.localStorage.get(doc.slug); + if (!Array.isArray(cached)) { + return; } - const keys = app.localStorage - .keys() - .filter((key) => !DB.NOT_INDEXES.includes(key)); - - if (keys.length === 0) { + app.localStorage.del(doc.slug); + if (cached[0] !== mtime) { return; } - this.db((db) => { - let store; - try { - store = this.indexesStore(db, "readwrite"); - } catch (error) { - // Wait for the store rather than sweeping without one, which would - // drop every index instead of moving it. - if (this.onMissingIndexesStore(error) && _retry) { - setTimeout(() => this.migrateIndexes(false), 0); - return; - } - } - - // One at a time, rather than reading every index into memory first. - for (var key of keys) { - const cached = app.localStorage.get(key); - if (store && isCachedIndex(cached)) { - try { - store.put(cached, key); - } catch (error) {} - } - app.localStorage.del(key); - } - }); - } - - /** - * @param {IDBDatabase | undefined} db - * @param {IDBTransactionMode} mode - * @returns {IDBObjectStore} Throws when the database is missing, or hasn't - * got the store yet, which every caller treats as a cache miss. - */ - indexesStore(db, mode) { - return this.idbTransaction(db, { - stores: [DB.INDEXES_STORE], - mode, - }).objectStore(DB.INDEXES_STORE); - } - - /** - * @param {{ name?: string }} error - * @returns {boolean} Whether the store was the thing that was missing, in - * which case the schema has been bumped so that the next open creates it. - */ - onMissingIndexesStore(error) { - if (error?.name !== "NotFoundError") { - return false; - } - this.migrate(); - return true; + this.storeIndex(doc, mtime, cached[1]); + return cached[1]; } /** @@ -971,11 +918,3 @@ export class DB { return app.settings.get("schema"); } } - -/** - * @param {unknown} value - * @returns {boolean} Whether `value` is an index as the old localStorage cache - * stored it: the mtime it was fetched at, and the index itself. - */ -const isCachedIndex = (value) => - Array.isArray(value) && value.length === 2 && typeof value[0] === "number"; diff --git a/assets/javascripts/collections/docs.js b/assets/javascripts/collections/docs.js index 6c66f9fda2..b2cc8c8a4a 100644 --- a/assets/javascripts/collections/docs.js +++ b/assets/javascripts/collections/docs.js @@ -93,13 +93,6 @@ export class Docs extends Collection { } } - /** Drops every doc's cached index. */ - clearCache() { - for (var doc of this.models) { - doc.clearCache(); - } - } - /** * Removes every doc's offline database, one at a time. * diff --git a/assets/javascripts/lib/local_storage_store.js b/assets/javascripts/lib/local_storage_store.js index 95448ab2c3..ee28bc16bf 100644 --- a/assets/javascripts/lib/local_storage_store.js +++ b/assets/javascripts/lib/local_storage_store.js @@ -8,7 +8,6 @@ * @property {(key: string) => unknown} get * @property {(key: string, value: unknown) => boolean | undefined} set * @property {(key: string) => boolean | undefined} del - * @property {() => string[]} keys * @property {() => boolean | undefined} reset */ @@ -53,17 +52,6 @@ export const LocalStorageStore = class LocalStorageStore { } catch (error) {} } - /** - * @returns {string[]} Every key, or an empty list when storage is unreadable. - */ - keys() { - try { - return Object.keys(localStorage); - } catch (error) { - return []; - } - } - /** * @returns {boolean | undefined} `true` when cleared, `undefined` when it failed. */ diff --git a/assets/javascripts/lib/page.js b/assets/javascripts/lib/page.js index afbaf17544..c2de6ac442 100644 --- a/assets/javascripts/lib/page.js +++ b/assets/javascripts/lib/page.js @@ -1,6 +1,6 @@ import { app } from "../app/app.js"; import { config } from "../app/config.js"; -import { settingsStore } from "./settings_store.js"; +import { expireCookie, settingsStore } from "./settings_store.js"; import { $ } from "./util.js"; import { Notif } from "../views/misc/notif.js"; @@ -562,7 +562,7 @@ export const resetAnalytics = function () { for (var cookie of document.cookie.split(/;\s?/)) { var name = cookie.split("=")[0]; if (name[0] === "_" && name[1] !== "_") { - document.cookie = `${name}=;path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT`; + expireCookie(name); } } }; diff --git a/assets/javascripts/lib/settings_store.js b/assets/javascripts/lib/settings_store.js index d2299bab84..cf4ccfd681 100644 --- a/assets/javascripts/lib/settings_store.js +++ b/assets/javascripts/lib/settings_store.js @@ -25,8 +25,6 @@ export class SettingsStore { /** The localStorage key everything is stored under. */ static KEY = "settings"; - static INT = /^\d+$/; - /** * Hook called when a value read back after a write doesn't match what was * written. Replaced by the app at boot; a no-op by default. @@ -37,16 +35,6 @@ export class SettingsStore { */ static onBlocked(key, value, actual) {} - /** - * @param {SettingValue} value - * @returns {SettingValue} `value`, as a number when it is all digits. - */ - static parse(value) { - return value != null && SettingsStore.INT.test(String(value)) - ? parseInt(String(value), 10) - : value; - } - /** Opens the store and takes in whatever is still in cookies. */ constructor() { this.storage = new LocalStorageStore(); @@ -58,7 +46,7 @@ export class SettingsStore { * @returns {SettingValue} The stored value, as a number when it is all digits. */ get(key) { - return SettingsStore.parse(this.dump()[key]); + return parse(this.dump()[key]); } /** @@ -76,11 +64,11 @@ export class SettingsStore { if (value === true) { value = 1; } - value = SettingsStore.parse(value); + value = parse(value); const settings = this.dump(); settings[key] = "" + value; - this.write(settings); + this.storage.set(SettingsStore.KEY, settings); const actual = this.get(key); if (actual !== value) { @@ -92,7 +80,7 @@ export class SettingsStore { del(key) { const settings = this.dump(); delete settings[key]; - this.write(settings); + this.storage.set(SettingsStore.KEY, settings); } /** Clears every setting. */ @@ -111,46 +99,61 @@ export class SettingsStore { : {}; } - /** @param {Record} settings */ - write(settings) { - this.storage.set(SettingsStore.KEY, settings); - } - /** * Takes in the settings an earlier version of the app left in cookies, and * expires them. Does nothing once they are gone. * * What is already stored wins, being the newer of the two. Cookies with a * single leading underscore belong to the analytics vendors, not to us. + * + * Remove once the app has had a release or two to empty the jar out. */ migrate() { - const settings = this.dump(); - let found = false; - // Reading document.cookie throws where cookies are turned off entirely. try { - for (var cookie of document.cookie.split(/;\s?/)) { - if (!cookie || 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 || ""); - found = true; - } - document.cookie = `${name}=;path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT`; + if (!document.cookie) { + return; + } + } catch (error) { + return; + } + + const settings = this.dump(); + + for (var cookie of document.cookie.split(/;\s?/)) { + if (cookie[0] === "_") { + continue; } - } catch (error) {} + const [name, value] = cookie.split("="); + const key = decode(name); - if (found) { - this.write(settings); + // 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); } } +/** + * Expires a cookie, whoever set it. + * + * @param {string} name + */ +export const expireCookie = (name) => { + document.cookie = `${name}=;path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT`; +}; + +/** + * @param {SettingValue} value + * @returns {SettingValue} `value`, as a number when it is all digits. + */ +const parse = (value) => + typeof value === "string" && /^\d+$/.test(value) ? parseInt(value, 10) : value; + /** * @param {string} value * @returns {string} `value`, decoded, or as it stands when it isn't valid diff --git a/assets/javascripts/models/doc.js b/assets/javascripts/models/doc.js index ff1264121b..b6579d731d 100644 --- a/assets/javascripts/models/doc.js +++ b/assets/javascripts/models/doc.js @@ -156,11 +156,7 @@ export class Doc extends Model { * @param {() => void} onError * @param {DocLoadOptions} [options] */ - load(onSuccess, onError, options) { - if (options == null) { - options = {}; - } - + load(onSuccess, onError, options = {}) { const fromNetwork = () => { ajax({ url: this.indexUrl(), @@ -168,7 +164,7 @@ export class Doc extends Model { this.reset(data); onSuccess(); if (options.writeCache) { - this._setCache(data); + app.db.storeIndex(this, this.mtime, data); } }, error: onError, @@ -180,7 +176,9 @@ export class Doc extends Model { return; } - this._getCache((data) => { + // A cache hit calls back asynchronously and a miss goes to the network, so + // `onSuccess` never runs before this returns. + app.db.loadIndex(this, this.mtime, (data) => { if (data) { this.reset(data); onSuccess(); @@ -190,25 +188,6 @@ export class Doc extends Model { }); } - /** Drops the cached index. */ - clearCache() { - app.db.deleteIndex(this); - } - - /** - * @param {(index?: unknown) => void} fn Called with the cached index, or with - * nothing when it is missing or stale. A hit is always asynchronous, and a - * miss leads to the network, so `load` never calls back synchronously. - */ - _getCache(fn) { - app.db.loadIndex(this, this.mtime, fn); - } - - /** @param {unknown} data */ - _setCache(data) { - app.db.storeIndex(this, this.mtime, data); - } - /** * Downloads the doc's database and stores it offline. Does nothing while an * install or uninstall is already running. diff --git a/test/assets/doc_cache_test.js b/test/assets/doc_cache_test.js index 76fe065c98..95240fc158 100644 --- a/test/assets/doc_cache_test.js +++ b/test/assets/doc_cache_test.js @@ -4,7 +4,6 @@ import assert from "node:assert/strict"; import test from "node:test"; import { app } from "../../assets/javascripts/app/app.js"; -import { LocalStorageStore } from "../../assets/javascripts/lib/local_storage_store.js"; import { DB } from "../../assets/javascripts/app/db.js"; import { Doc } from "../../assets/javascripts/models/doc.js"; @@ -34,6 +33,10 @@ Object.defineProperty(globalThis, "XMLHttpRequest", { configurable: true, }); +test.beforeEach(() => { + requests.length = 0; +}); + /** The index a doc loads, kept minimal: the shape is all Doc#reset touches. */ const INDEX = { entries: [], types: [] }; @@ -60,7 +63,6 @@ const stubStore = (cached) => { const newDoc = () => new Doc({ name: "CSS", slug: "css", mtime: 42 }); test("a cached index is used instead of the network", async () => { - requests.length = 0; const calls = stubStore(INDEX); const doc = newDoc(); @@ -76,7 +78,6 @@ test("a cached index is used instead of the network", async () => { }); test("a miss falls through to the network, and stores what it fetched", async () => { - requests.length = 0; const calls = stubStore(undefined); const doc = newDoc(); @@ -99,7 +100,6 @@ test("a miss falls through to the network, and stores what it fetched", async () }); test("the cache is left alone when the caller doesn't ask for it", async () => { - requests.length = 0; const calls = stubStore(INDEX); const doc = newDoc(); @@ -116,65 +116,34 @@ test("the cache is left alone when the caller doesn't ask for it", async () => { assert.deepEqual(calls.written, [], "and nothing should have been written"); }); -test("a doc drops its cached index when it is cleared", () => { - const calls = stubStore(INDEX); - newDoc().clearCache(); - assert.deepEqual(calls.written, [["css", null]]); -}); - -// Items are own properties of a real Storage, and its methods live on the -// prototype, which is what makes Object.keys() return the keys and nothing -// else. -class FakeStorage { - constructor(/** @type {Record} */ entries) { - Object.assign(this, entries); - } - getItem(/** @type {string} */ key) { - return Object.prototype.hasOwnProperty.call(this, key) ? this[key] : null; - } - setItem(/** @type {string} */ key, /** @type {string} */ value) { - this[key] = String(value); - } - removeItem(/** @type {string} */ key) { - delete this[key]; - } - clear() { - for (const key of Object.keys(this)) delete this[key]; - } -} - -test("the indexes an older app cached in localStorage are taken in", () => { - const stored = new FakeStorage({ - settings: '{"docs":"css"}', - "override-mobile-detect": "true", - css: '[42,{"entries":[]}]', - html: '[7,{"entries":[]}]', - junk: '"not an index"', +test("an index left in localStorage is taken in on the read that wants it", () => { + const stored = { css: [42, INDEX], html: [7, INDEX] }; + app.localStorage = /** @type {any} */ ({ + get: (/** @type {string} */ key) => stored[key], + del: (/** @type {string} */ key) => delete stored[key], }); - Object.defineProperty(globalThis, "localStorage", { - value: stored, - writable: true, - configurable: true, - }); - - app.localStorage = new LocalStorageStore(); const db = new DB(); /** @type {unknown[]} */ const puts = []; - db.db = (fn) => fn(/** @type {any} */ ({})); - db.indexesStore = () => - /** @type {any} */ ({ put: (value, key) => puts.push([key, value]) }); - - db.migrateIndexes(); - - assert.deepEqual(puts, [ - ["css", [42, { entries: [] }]], - ["html", [7, { entries: [] }]], - ]); - assert.deepEqual( - Object.keys(stored), - ["settings", "override-mobile-detect"], - "everything else should have been cleared out", - ); + db.indexes = (mode, fn) => + fn(/** @type {any} */ ({ put: (value, key) => puts.push([key, value]) })); + + assert.equal(db.importIndex(newDoc(), 42), INDEX, "the doc's own index"); + assert.deepEqual(puts, [["css", [42, INDEX]]], "moved into the database"); + assert.deepEqual(stored, { html: [7, INDEX] }, "and out of localStorage"); +}); + +test("an index left over from an earlier build is dropped, not taken in", () => { + const stored = { css: [7, INDEX] }; + app.localStorage = /** @type {any} */ ({ + get: (/** @type {string} */ key) => stored[key], + del: (/** @type {string} */ key) => delete stored[key], + }); + + const db = new DB(); + db.indexes = () => assert.fail("nothing should be stored"); + + assert.equal(db.importIndex(newDoc(), 42), undefined); + assert.deepEqual(stored, {}); }); diff --git a/test/assets/settings_store_test.js b/test/assets/settings_store_test.js index 70258ecd23..6d2c0b67ae 100644 --- a/test/assets/settings_store_test.js +++ b/test/assets/settings_store_test.js @@ -32,17 +32,13 @@ const jar = new Map(); Object.defineProperty(document, "cookie", { get: () => [...jar].map(([key, value]) => `${key}=${value}`).join("; "), set: (/** @type {string} */ string) => { - const [pair, ...attributes] = string.split(";"); + const pair = string.split(";")[0]; const separator = pair.indexOf("="); - const key = pair.slice(0, separator); - const expires = attributes - .map((attribute) => attribute.trim()) - .find((attribute) => /^expires=/i.test(attribute)); - - if (expires && new Date(expires.slice(8)) <= new Date()) { - jar.delete(key); + // The only expiry anything writes is the epoch, which deletes. + if (/;\s*expires=/i.test(string)) { + jar.delete(pair.slice(0, separator)); } else { - jar.set(key, pair.slice(separator + 1)); + jar.set(pair.slice(0, separator), pair.slice(separator + 1)); } }, configurable: true, @@ -96,56 +92,29 @@ test("sees what another store has written", () => { test("takes in the settings left in cookies, and expires them", () => { reset(); + // What is stored already wins over the cookie of the same name; the vendors' + // own cookies aren't ours to take; the session-only one doesn't become + // permanent; and a value keeps its spaces rather than its escapes. + new SettingsStore().set("theme", "dark"); document.cookie = "docs=css/javascript"; document.cookie = "size=320"; - - const store = new SettingsStore(); - - assert.equal(store.get("docs"), "css/javascript"); - assert.equal(store.get("size"), 320); - assert.equal(document.cookie, "", "the cookies should be gone"); -}); - -test("decodes a cookie value rather than storing its escapes", () => { - reset(); document.cookie = "layout=_max-width%20_sidebar-hidden"; - - assert.equal( - new SettingsStore().get("layout"), - "_max-width _sidebar-hidden", - ); -}); - -test("keeps the stored value when a cookie of the same name is left over", () => { - reset(); - new SettingsStore().set("theme", "dark"); document.cookie = "theme=default"; - - assert.equal(new SettingsStore().get("theme"), "dark"); - assert.equal(document.cookie, ""); -}); - -test("leaves the analytics vendors' own cookies alone", () => { - reset(); + document.cookie = "analyticsConsentAsked=1"; document.cookie = "_ga=GA1.2.3"; const store = new SettingsStore(); - assert.deepEqual(store.dump(), {}); + assert.deepEqual(store.dump(), { + theme: "dark", + docs: "css/javascript", + size: "320", + layout: "_max-width _sidebar-hidden", + }); + assert.equal(store.get("size"), 320, "and integers still parse"); assert.equal(document.cookie, "_ga=GA1.2.3"); }); -test("drops the consent-asked cookie rather than making it permanent", () => { - reset(); - document.cookie = "analyticsConsentAsked=1"; - document.cookie = "analyticsConsent=1"; - - const store = new SettingsStore(); - - assert.deepEqual(store.dump(), { analyticsConsent: "1" }); - assert.equal(document.cookie, ""); -}); - test("reports a write that doesn't stick", () => { reset(); const store = new SettingsStore(); diff --git a/views/service-worker.js.erb b/views/service-worker.js.erb index d6fe4b493c..1f73e19451 100644 --- a/views/service-worker.js.erb +++ b/views/service-worker.js.erb @@ -34,6 +34,8 @@ self.addEventListener('fetch', event => { const cachedResponse = await caches.match(event.request); if (cachedResponse) return cachedResponse; + const url = new URL(event.request.url); + try { const response = await fetch(event.request); @@ -42,7 +44,6 @@ self.addEventListener('fetch', event => { <%# enabled docs into this file at request time. index.json is served %> <%# from the docs CDN (a different origin in production), which sends CORS %> <%# headers, so the response is inspectable and cacheable — no origin check. %> - const url = new URL(event.request.url); if ( event.request.method === 'GET' && url.pathname.endsWith('/index.json') && @@ -54,8 +55,6 @@ self.addEventListener('fetch', event => { return response; } catch (err) { - const url = new URL(event.request.url); - const pathname = url.pathname; const filename = pathname.substr(1 + pathname.lastIndexOf('/')).split(/\#|\?/g)[0]; const extensions = ['.html', '.css', '.js', '.json', '.png', '.ico', '.svg', '.xml']; From 17f79a9e094e417a26a220950813de5873ce169a Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:48:25 +0200 Subject: [PATCH 06/12] Keep the index cache callback asynchronous 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. --- assets/javascripts/app/db.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/assets/javascripts/app/db.js b/assets/javascripts/app/db.js index d47298623d..cab95af0c0 100644 --- a/assets/javascripts/app/db.js +++ b/assets/javascripts/app/db.js @@ -528,13 +528,17 @@ export class DB { * @param {number} mtime The build to read it for; one cached for an earlier * build is passed over, and overwritten when the doc is fetched again. * @param {(index?: unknown) => void} fn Called with the index, or with - * nothing when there isn't a usable one. + * nothing when there isn't a usable one. Never before `loadIndex` returns; + * Doc#load and its callers rely on it. */ loadIndex(doc, mtime, fn) { this.indexes("readonly", (store) => { const req = store?.get(doc.slug); if (!req) { - fn(this.importIndex(doc, mtime)); + // `db` runs its callback there and then when IndexedDB is off, and + // Docs#load can't be called back before `Doc#load` has returned. + const index = this.importIndex(doc, mtime); + setTimeout(() => fn(index), 0); return; } From d475e3c6a9dcb2b432adbcc2d278edb180e61630 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:48:44 +0200 Subject: [PATCH 07/12] Read the mobile override where the migration puts it 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. --- assets/javascripts/views/layout/mobile.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/assets/javascripts/views/layout/mobile.js b/assets/javascripts/views/layout/mobile.js index 4071750870..578df60b3b 100644 --- a/assets/javascripts/views/layout/mobile.js +++ b/assets/javascripts/views/layout/mobile.js @@ -2,6 +2,7 @@ import { app } from "../../app/app.js"; import { page } from "../../lib/page.js"; +import { settingsStore } from "../../lib/settings_store.js"; import { $ } from "../../lib/util.js"; import { ListFold } from "../list/list_fold.js"; import { ListSelect } from "../list/list_select.js"; @@ -28,12 +29,13 @@ export class Mobile extends View { /** * @returns {boolean} Whether to use the phone layout. The user agent is * consulted as well as the viewport, because some devices report a - * desktop-sized width. + * desktop-sized width. `override-mobile-detect` settles it either way; + * it is set by hand, and was a cookie before the settings moved. */ static detect() { - const override = app.localStorage.get("override-mobile-detect"); + const override = settingsStore.get("override-mobile-detect"); if (override != null) { - return !!override; + return override !== 0 && override !== "false"; } try { return ( From 0d2959bc7d2d9dd9ae09ec614e93e8755d200e7d Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:49:43 +0200 Subject: [PATCH 08/12] Expire the settings cookies only once they are stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- assets/javascripts/lib/settings_store.js | 17 +++++++++++++++-- test/assets/settings_store_test.js | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/assets/javascripts/lib/settings_store.js b/assets/javascripts/lib/settings_store.js index cf4ccfd681..e00fb0839b 100644 --- a/assets/javascripts/lib/settings_store.js +++ b/assets/javascripts/lib/settings_store.js @@ -105,6 +105,8 @@ export class SettingsStore { * * What is already stored wins, being the newer of the two. Cookies with a * single leading underscore belong to the analytics vendors, not to us. + * Nothing is expired unless the write lands, or a browser that won't take + * the settings would be left with no copy of them at all. * * Remove once the app has had a release or two to empty the jar out. */ @@ -119,6 +121,7 @@ export class SettingsStore { } const settings = this.dump(); + const names = []; for (var cookie of document.cookie.split(/;\s?/)) { if (cookie[0] === "_") { @@ -131,10 +134,20 @@ export class SettingsStore { if (key !== "analyticsConsentAsked" && !(key in settings)) { settings[key] = decode(value || ""); } - expireCookie(name); + names.push(name); } - this.storage.set(SettingsStore.KEY, settings); + if (!names.length) { + return; + } + + // The jar is the only copy until the write lands, and LocalStorageStore + // reports a write it couldn't make rather than throwing. + if (!this.storage.set(SettingsStore.KEY, settings)) { + return; + } + + names.forEach(expireCookie); } } diff --git a/test/assets/settings_store_test.js b/test/assets/settings_store_test.js index 6d2c0b67ae..30e31d89c8 100644 --- a/test/assets/settings_store_test.js +++ b/test/assets/settings_store_test.js @@ -133,3 +133,22 @@ test("reports a write that doesn't stick", () => { SettingsStore.onBlocked = onBlocked; } }); + +test("leaves the cookies alone when the migration can't be written", () => { + reset(); + document.cookie = "docs=css/javascript"; + document.cookie = "theme=dark"; + + storageWritable = false; + new SettingsStore(); + + assert.equal(document.cookie, "docs=css/javascript; theme=dark"); + + // And a later boot, with storage writable again, still finds them. + storageWritable = true; + assert.deepEqual(new SettingsStore().dump(), { + docs: "css/javascript", + theme: "dark", + }); + assert.equal(document.cookie, ""); +}); From 39d8e3114cc5d2339c122a7238a8dfca524b00e7 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:51:00 +0200 Subject: [PATCH 09/12] Drop a migrated index from localStorage once it is stored 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. --- assets/javascripts/app/db.js | 22 ++++++++++++++++++---- test/assets/doc_cache_test.js | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/assets/javascripts/app/db.js b/assets/javascripts/app/db.js index cab95af0c0..31b1ec993d 100644 --- a/assets/javascripts/app/db.js +++ b/assets/javascripts/app/db.js @@ -557,9 +557,19 @@ export class DB { * @param {Doc} doc * @param {number} mtime The build the index was fetched for. * @param {unknown} index + * @param {() => void} [fn] Called once the write has been committed, and not + * at all when there was nowhere to write it. */ - storeIndex(doc, mtime, index) { - this.indexes("readwrite", (store) => store?.put([mtime, index], doc.slug)); + storeIndex(doc, mtime, index, fn) { + this.indexes("readwrite", (store) => { + if (!store) { + return; + } + store.put([mtime, index], doc.slug); + if (fn) { + store.transaction.oncomplete = fn; + } + }); } /** @param {Doc} doc */ @@ -589,12 +599,16 @@ export class DB { return; } - app.localStorage.del(doc.slug); if (cached[0] !== mtime) { + app.localStorage.del(doc.slug); return; } - this.storeIndex(doc, mtime, cached[1]); + // localStorage holds the only copy until the write lands, and there may be + // nowhere to write it yet: a database that predates the indexes store only + // queues its schema bump when it first misses it, and a browser without + // IndexedDB never has one. + this.storeIndex(doc, mtime, cached[1], () => app.localStorage.del(doc.slug)); return cached[1]; } diff --git a/test/assets/doc_cache_test.js b/test/assets/doc_cache_test.js index 95240fc158..25c59e9455 100644 --- a/test/assets/doc_cache_test.js +++ b/test/assets/doc_cache_test.js @@ -126,12 +126,41 @@ test("an index left in localStorage is taken in on the read that wants it", () = const db = new DB(); /** @type {unknown[]} */ const puts = []; + /** @type {any} */ + const transaction = {}; db.indexes = (mode, fn) => - fn(/** @type {any} */ ({ put: (value, key) => puts.push([key, value]) })); + fn( + /** @type {any} */ ({ + put: (value, key) => puts.push([key, value]), + transaction, + }), + ); assert.equal(db.importIndex(newDoc(), 42), INDEX, "the doc's own index"); assert.deepEqual(puts, [["css", [42, INDEX]]], "moved into the database"); - assert.deepEqual(stored, { html: [7, INDEX] }, "and out of localStorage"); + assert.deepEqual( + stored, + { css: [42, INDEX], html: [7, INDEX] }, + "and kept until the write is committed", + ); + + transaction.oncomplete(); + assert.deepEqual(stored, { html: [7, INDEX] }, "and dropped once it is"); +}); + +test("an index left in localStorage is kept when there is nowhere to put it", () => { + const stored = { css: [42, INDEX] }; + app.localStorage = /** @type {any} */ ({ + get: (/** @type {string} */ key) => stored[key], + del: (/** @type {string} */ key) => delete stored[key], + }); + + // No store: no IndexedDB at all, or a database that has yet to be given one. + const db = new DB(); + db.indexes = (mode, fn) => fn(undefined); + + assert.equal(db.importIndex(newDoc(), 42), INDEX); + assert.deepEqual(stored, { css: [42, INDEX] }, "the only copy is kept"); }); test("an index left over from an earlier build is dropped, not taken in", () => { From 17c6bcc89930c103c916a26208e702d103a3cbe4 Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:51:25 +0200 Subject: [PATCH 10/12] Keep the worker alive until a cached index is committed 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. --- views/service-worker.js.erb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/views/service-worker.js.erb b/views/service-worker.js.erb index 1f73e19451..0943237237 100644 --- a/views/service-worker.js.erb +++ b/views/service-worker.js.erb @@ -49,8 +49,12 @@ self.addEventListener('fetch', event => { url.pathname.endsWith('/index.json') && response.ok ) { - const cache = await caches.open(cacheName); - cache.put(event.request, response.clone()); + <%# waitUntil rather than await: the response goes back without %> + <%# waiting on the write, but the worker stays alive until it lands. %> + const copy = response.clone(); + event.waitUntil( + caches.open(cacheName).then(cache => cache.put(event.request, copy)), + ); } return response; From abe98d91e53078be965293b6529d172e2e97250b Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:52:42 +0200 Subject: [PATCH 11/12] Reload once the index cache is actually empty 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. --- assets/javascripts/app/app.js | 13 +++++----- assets/javascripts/app/db.js | 33 ++++++++++++++++++++++--- test/assets/doc_cache_test.js | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/assets/javascripts/app/app.js b/assets/javascripts/app/app.js index 9af2b7c5fe..7dcd06bdf1 100644 --- a/assets/javascripts/app/app.js +++ b/assets/javascripts/app/app.js @@ -414,12 +414,13 @@ export class App extends Events { /** Drops the cached indexes and reloads the app. */ reload() { - this.db.clearIndexes(); - if (this.serviceWorker) { - this.serviceWorker.reload(); - } else { - this.reboot(); - } + this.db.clearIndexes(() => { + if (this.serviceWorker) { + this.serviceWorker.reload(); + } else { + this.reboot(); + } + }); } /** Clears every trace of the app and returns to the index. */ diff --git a/assets/javascripts/app/db.js b/assets/javascripts/app/db.js index 31b1ec993d..89919eee05 100644 --- a/assets/javascripts/app/db.js +++ b/assets/javascripts/app/db.js @@ -577,9 +577,36 @@ export class DB { this.indexes("readwrite", (store) => store?.delete(doc.slug)); } - /** Drops every cached index. */ - clearIndexes() { - this.indexes("readwrite", (store) => store?.clear()); + /** + * Drops every cached index, including any an earlier version of the app left + * in localStorage and hasn't been asked for yet. + * + * @param {() => void} fn Called once they are gone — and called even when the + * transaction doesn't go through, so that a caller waiting to reload does. + */ + clearIndexes(fn) { + for (var doc of app.docs.all().concat(app.disabledDocs.all())) { + app.localStorage.del(doc.slug); + } + + this.indexes("readwrite", (store) => { + if (!store) { + fn(); + return; + } + + store.clear(); + const txn = store.transaction; + const done = () => { + txn.oncomplete = txn.onerror = txn.onabort = null; + fn(); + }; + txn.oncomplete = done; + txn.onerror = txn.onabort = (event) => { + event.preventDefault(); + done(); + }; + }); } /** diff --git a/test/assets/doc_cache_test.js b/test/assets/doc_cache_test.js index 25c59e9455..c5ed8fbbae 100644 --- a/test/assets/doc_cache_test.js +++ b/test/assets/doc_cache_test.js @@ -176,3 +176,48 @@ test("an index left over from an earlier build is dropped, not taken in", () => assert.equal(db.importIndex(newDoc(), 42), undefined); assert.deepEqual(stored, {}); }); + +/** Stands in for the doc collections, which name the legacy localStorage keys. */ +const stubDocs = (...slugs) => { + app.docs = /** @type {any} */ ({ all: () => slugs.map((slug) => ({ slug })) }); + app.disabledDocs = /** @type {any} */ ({ all: () => [] }); +}; + +test("clearing the cache empties the store and what was left in localStorage", () => { + const stored = { css: [42, INDEX], html: [7, INDEX] }; + app.localStorage = /** @type {any} */ ({ + get: (/** @type {string} */ key) => stored[key], + del: (/** @type {string} */ key) => delete stored[key], + }); + stubDocs("css", "html"); + + const db = new DB(); + /** @type {any} */ + const transaction = {}; + let cleared = false; + db.indexes = (mode, fn) => + fn(/** @type {any} */ ({ clear: () => (cleared = true), transaction })); + + let done = false; + db.clearIndexes(() => (done = true)); + + assert.deepEqual(stored, {}, "the leftovers go"); + assert.ok(cleared, "the store is cleared"); + assert.equal(done, false, "and the caller waits for the transaction"); + + transaction.oncomplete(); + assert.ok(done, "which it is told about"); +}); + +test("clearing the cache calls back even with no store to clear", () => { + app.localStorage = /** @type {any} */ ({ del: () => {} }); + stubDocs(); + + const db = new DB(); + db.indexes = (mode, fn) => fn(undefined); + + let done = false; + db.clearIndexes(() => (done = true)); + + assert.ok(done, "or a reload waiting on it would never happen"); +}); From c1df73848439dda59fb4598404347d8543c0fbab Mon Sep 17 00:00:00 2001 From: Simon Legner Date: Mon, 14 Sep 2026 19:53:45 +0200 Subject: [PATCH 12/12] Carry the consent prompt's session flag across the migration 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. --- assets/javascripts/lib/page.js | 10 +++++--- assets/javascripts/lib/settings_store.js | 20 +++++++++++++--- test/assets/settings_store_test.js | 30 ++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/assets/javascripts/lib/page.js b/assets/javascripts/lib/page.js index c2de6ac442..d3af80f296 100644 --- a/assets/javascripts/lib/page.js +++ b/assets/javascripts/lib/page.js @@ -1,6 +1,10 @@ import { app } from "../app/app.js"; import { config } from "../app/config.js"; -import { expireCookie, settingsStore } from "./settings_store.js"; +import { + SettingsStore, + expireCookie, + settingsStore, +} from "./settings_store.js"; import { $ } from "./util.js"; import { Notif } from "../views/misc/notif.js"; @@ -546,10 +550,10 @@ var track = function () { */ var consentAsked = function () { try { - if (sessionStorage.getItem("analyticsConsentAsked")) { + if (sessionStorage.getItem(SettingsStore.ASKED_KEY)) { return true; } - sessionStorage.setItem("analyticsConsentAsked", "1"); + sessionStorage.setItem(SettingsStore.ASKED_KEY, "1"); } catch (error) {} return false; }; diff --git a/assets/javascripts/lib/settings_store.js b/assets/javascripts/lib/settings_store.js index e00fb0839b..5b2250f468 100644 --- a/assets/javascripts/lib/settings_store.js +++ b/assets/javascripts/lib/settings_store.js @@ -25,6 +25,12 @@ export class SettingsStore { /** The localStorage key everything is stored under. */ static KEY = "settings"; + /** + * The sessionStorage key the once-a-session analytics prompt is tracked by. + * Not a setting: it was a session cookie, and is meant to last a visit. + */ + static ASKED_KEY = "analyticsConsentAsked"; + /** * Hook called when a value read back after a write doesn't match what was * written. Replaced by the app at boot; a no-op by default. @@ -83,9 +89,12 @@ export class SettingsStore { this.storage.set(SettingsStore.KEY, settings); } - /** Clears every setting. */ + /** Clears every setting, and the flags that outlive them. */ reset() { this.storage.del(SettingsStore.KEY); + try { + sessionStorage.removeItem(SettingsStore.ASKED_KEY); + } catch (error) {} } /** @@ -130,8 +139,13 @@ export class SettingsStore { const [name, value] = cookie.split("="); const key = decode(name); - // analyticsConsentAsked was a session cookie, and is sessionStorage now. - if (key !== "analyticsConsentAsked" && !(key in settings)) { + if (key === SettingsStore.ASKED_KEY) { + // It was a session cookie, and is sessionStorage now. Carried over, or + // the prompt would come back on the visit that upgrades. + try { + sessionStorage.setItem(key, "1"); + } catch (error) {} + } else if (!(key in settings)) { settings[key] = decode(value || ""); } names.push(name); diff --git a/test/assets/settings_store_test.js b/test/assets/settings_store_test.js index 30e31d89c8..2d2678c859 100644 --- a/test/assets/settings_store_test.js +++ b/test/assets/settings_store_test.js @@ -24,6 +24,21 @@ Object.defineProperty(globalThis, "localStorage", { configurable: true, }); +/** @type {Map} */ +const session = new Map(); + +Object.defineProperty(globalThis, "sessionStorage", { + value: { + getItem: (/** @type {string} */ key) => + session.has(key) ? session.get(key) : null, + setItem: (/** @type {string} */ key, /** @type {string} */ value) => + session.set(key, String(value)), + removeItem: (/** @type {string} */ key) => session.delete(key), + }, + writable: true, + configurable: true, +}); + // A cookie jar the migration can read and expire: keys and values stay // percent-encoded, as they are on a real `document.cookie`. /** @type {Map} */ @@ -48,6 +63,7 @@ Object.defineProperty(document, "cookie", { const reset = () => { jar.clear(); storage.clear(); + session.clear(); storageWritable = true; }; @@ -113,6 +129,20 @@ test("takes in the settings left in cookies, and expires them", () => { }); assert.equal(store.get("size"), 320, "and integers still parse"); assert.equal(document.cookie, "_ga=GA1.2.3"); + assert.equal( + sessionStorage.getItem("analyticsConsentAsked"), + "1", + "the session-only one carries over to sessionStorage", + ); +}); + +test("a reset takes the once-a-session flags with it", () => { + reset(); + sessionStorage.setItem("analyticsConsentAsked", "1"); + + new SettingsStore().reset(); + + assert.equal(sessionStorage.getItem("analyticsConsentAsked"), null); }); test("reports a write that doesn't stick", () => {