diff --git a/eslint.config.mjs b/eslint.config.mjs index f86f1dd7..ee66f1df 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -63,6 +63,33 @@ const CORRECTNESS = { "no-async-promise-executor": "error", }; +// Where site data is blocked, touching either storage global throws rather than +// answering a storage that refuses, and the name alone is enough: at module +// scope in `web/interview.js` it stopped the whole page from loading (d5ad668), +// and as an argument it throws before the callee's own `try` can answer it. Both +// work wherever storage is allowed, so a local run never shows either failure. +const STORAGE_MESSAGE = + 'Throws where site data is blocked. Use storageArea() from "./audio-output.js".'; +const STORAGE_ACCESS = { + "no-restricted-globals": [ + "error", + ...["localStorage", "sessionStorage"].map((name) => ({ + name, + message: STORAGE_MESSAGE, + })), + ], + "no-restricted-properties": [ + "error", + ...["window", "self", "globalThis"].flatMap((object) => + ["localStorage", "sessionStorage"].map((property) => ({ + object, + property, + message: STORAGE_MESSAGE, + })), + ), + ], +}; + export default [ { // `externals/` holds unrelated checkouts, is untracked, and is excluded in @@ -84,6 +111,19 @@ export default [ linterOptions: { reportUnusedDisableDirectives: "error" }, rules: CORRECTNESS, }, + { + // Application code only. The review workbench under `.claude/skills` is a + // developer tool, not a page a candidate opens. + // + // `web/peer-lobby.js` is exempt for now. Three of its five + // references sit outside any `try`, one of them at module scope, and a + // mechanical `storageArea(...).setItem` would pass this rule and still throw + // on null. The fix needs a decision about where the invite token lives + // without storage, after the page has removed it from the URL. + files: ["web/**/*.js"], + ignores: ["web/peer-lobby.js"], + rules: STORAGE_ACCESS, + }, { // Node: the browser tests and the generator scripts. // diff --git a/tests/browser/account.test.js b/tests/browser/account.test.js index 39a257e3..49b51602 100644 --- a/tests/browser/account.test.js +++ b/tests/browser/account.test.js @@ -309,7 +309,7 @@ test("optional interview profile is accessible, bounded, and omitted when blank" // The focus travels in session storage, never the address bar. assert.match( app, - /storeSharedFocus\(sessionStorage, focus\?\.weakness \?\? null\)/, + /const tabStorage = storageArea\("sessionStorage"\)[\s\S]*storeSharedFocus\(tabStorage, focus\?\.weakness \?\? null\)/, ); assert.doesNotMatch(app, /searchParams\.set\("focus"/); assert.match(interview, /practiceFocus: consumeSharedFocus\(tabStorage\)/); @@ -337,7 +337,10 @@ test("document grounding is explicit, clearable, ephemeral, and absent from save app, /nodes\.groundingClear\.addEventListener\("click", clearGrounding\)/, ); - assert.match(app, /storeGroundingPacket\(sessionStorage, packet\)/); + assert.match( + app, + /const tabStorage = storageArea\("sessionStorage"\)[\s\S]*storeGroundingPacket\(tabStorage, packet\)/, + ); assert.match(interview, /consumeGroundingPacket\(tabStorage\)/); const savedArtifacts = [ read("history.js"), @@ -1217,6 +1220,38 @@ test("a report the account kept stays on this device too", async () => { ); }); +// A device copy that cannot be reached is a failed delete, not a missing report: +// the lobby tells the candidate what these answers say. +test("deleting a report says so when local storage cannot be reached", async () => { + const original = Object.getOwnPropertyDescriptor(globalThis, "localStorage"); + Object.defineProperty(globalThis, "localStorage", { + configurable: true, + get() { + throw new DOMException("blocked", "SecurityError"); + }, + }); + try { + assert.equal( + await deleteReport("keep", { + fetcher: async () => response({ signedIn: false }), + }), + "failed", + ); + assert.equal( + await deleteReport("keep", { + fetcher: async (url) => + url === "/api/session" + ? response({ signedIn: true }) + : response({ deleted: 1 }), + }), + "account-deleted-local-failed", + ); + } finally { + if (original) Object.defineProperty(globalThis, "localStorage", original); + else delete globalThis.localStorage; + } +}); + test("deleting an id no store holds says so instead of claiming a removal", async () => { // A pre-id row whose assigned id never reached storage: the rows are still // there, so answering "deleted" named a removal the next read undoes. diff --git a/tests/browser/lobby-render.test.js b/tests/browser/lobby-render.test.js index b1376782..25bee5de 100644 --- a/tests/browser/lobby-render.test.js +++ b/tests/browser/lobby-render.test.js @@ -19,7 +19,7 @@ // depends on: `document.querySelectorAll` answers ids and nothing else, so the // problem cards, the difficulty radios, the duration buttons and the loop // buttons are all absent, and this drives the page that is left. What that page -// still proves is the import, the signed-out account state, and the branch the +// still proves is the import, the unidentified account state, and the branch the // progress panel takes when it cannot read anything. import { test } from "node:test"; import assert from "node:assert/strict"; @@ -33,24 +33,28 @@ const dom = installDocument(markup); // work while it loads. Every test below reads the same rendered document. await import("/app.js"); -/// What the page may say to a signed-out visitor whose device has no history. +/// What the page may say to a visitor it cannot identify, on a device with no +/// history. /// /// Both of those are the stub's doing and neither is contrived: `installDocument` /// refuses the fetch the page starts at import, so the session lookup fails, and -/// plain `node` has no `localStorage`, so the saved-progress read fails too. -/// That is the worst case a real visitor can also reach, on a first visit with -/// the network down, and it is the one state this file can produce without a -/// browser. -const SIGNED_OUT = { - "account-status": "Signed out", +/// plain `node` has no storage globals, so the saved-progress read fails too. +/// That is the worst case a real visitor can also reach, with site data blocked +/// and the network down, and it is the one state this file can produce without a +/// browser. Start is held there, because a server that requires sign-in would +/// send the visitor straight back, so the page offers the account retry. +const UNIDENTIFIED = { + "account-status": "Could not check your account. Retry the account check.", + "login-link": "Retry account check", "progress-problem": "all", start: "Start interview", - "progress-summary": "Could not load progress saved on this device.", + "progress-summary": + "Could not load account progress. Retry the account check.", "practice-list-status": "0 problems in your list. Open the problem picker below to check problems. Kept for this visit only; browser storage is unavailable.", }; test("the lobby page loads outside a browser and says what it was given", () => { - for (const [id, expected] of Object.entries(SIGNED_OUT)) { + for (const [id, expected] of Object.entries(UNIDENTIFIED)) { assert.deepEqual(dom.node(id).spoken(), [expected], `#${id}`); } }); @@ -64,7 +68,7 @@ test("no other panel of the lobby speaks before anything has loaded", () => { const spoke = []; for (const [, id] of markup.matchAll(/id="([^"]+)"/g)) { const node = dom.node(id); - if (node && node.spoken().length && !(id in SIGNED_OUT)) spoke.push(id); + if (node && node.spoken().length && !(id in UNIDENTIFIED)) spoke.push(id); } assert.deepEqual( spoke, diff --git a/tests/browser/lobby.test.js b/tests/browser/lobby.test.js index 7bf2df76..45ecbdaf 100644 --- a/tests/browser/lobby.test.js +++ b/tests/browser/lobby.test.js @@ -561,6 +561,388 @@ lobbyTest( }, ); +// Where site data is blocked, reading the `sessionStorage` global throws. Start +// named the global in the arguments of its storage helpers, so the throw came +// before any helper could answer it, and the candidate was left on the lobby. +const blockSessionStorage = (page) => + page.addInitScript(() => { + Object.defineProperty(window, "sessionStorage", { + get() { + throw new DOMException("blocked", "SecurityError"); + }, + }); + }); + +lobbyTest("Practice starts when session storage is blocked", async (page) => { + await blockSessionStorage(page); + const picked = (await lobby(page)).card; + await page.click("#start"); + await page.waitForURL("**/interview?**"); + assert.equal(new URL(page.url()).searchParams.get("problem"), picked); +}); + +// Unlike grounding, a ticked focus does not stop Start: `storeSharedFocus` holds +// an unshared focus to be the safe outcome, and a refusal would land in the +// collapsed section, leaving a Start that looks dead. The address bar must not +// become the way round, since anyone can craft a link carrying that text. +lobbyTest( + "Practice starts without a shared focus when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + reports = [focusedAttempt(EASY[0])]; + const state = await lobby(page); + assert.match(state.focus, /Test boundaries/); + await page.check("#practice-focus-share-input"); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + assert.equal(new URL(page.url()).searchParams.get("problem"), state.card); + assert.doesNotMatch( + decodeURIComponent(page.url()).replaceAll("+", " "), + /Test boundaries/, + ); + }, +); + +// The packet can only reach the interview page through the tab's storage, so +// starting without it would drop what the candidate chose. Clearing is the way +// out the message offers, and it has to work with storage still blocked. +lobbyTest( + "Practice with grounding offers clearing it when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + await lobby(page); + await page.click("details.interview-context summary"); + await page.setInputFiles("#grounding-jd", { + name: "jd.txt", + mimeType: "text/plain", + buffer: Buffer.from("Must know Rust"), + }); + await page + .locator('#grounding-choices input[data-group="requirements"]') + .first() + .check(); + await page.check("#grounding-consent"); + await page.click("#start"); + await page.waitForFunction( + () => document.querySelector("#grounding-error").textContent !== "", + ); + assert.match( + await page.locator("#grounding-error").textContent(), + /Clear grounding to start normally/, + ); + assert.equal(new URL(page.url()).pathname, "/"); + await page.click("#grounding-clear"); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + }, +); + +// Where the block also refuses the session cookie, a recorded username would +// reach the interview signed out and come straight back with its choices reset. +// Phone-sized, because there Start sits far below the account row the reason +// lands in, and a reason out of view is a Start that looks dead. +lobbyTest( + "Start asks for site data before signing in when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let logins = 0; + page.on("request", (request) => { + if (new URL(request.url()).pathname === "/api/login") logins++; + }); + await lobby(page); + await page.fill("#github-login", "candidate"); + await page.click("details.interview-context summary"); + await page.setInputFiles("#grounding-jd", { + name: "jd.txt", + mimeType: "text/plain", + buffer: Buffer.from("Must know Rust"), + }); + await page + .locator('#grounding-choices input[data-group="requirements"]') + .first() + .check(); + await page.click("#start"); + assert.match( + await page.locator("#grounding-error").textContent(), + /Agree to send/, + ); + await page.check("#grounding-consent"); + await page.click("#start"); + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("Signing in needs site data"), + ); + assert.match( + await page.locator("#account-status").textContent(), + /Signing in needs site data/, + ); + // The refusal the candidate just answered does not stay beside the reason. + assert.equal(await page.locator("#grounding-error").textContent(), ""); + assert.equal( + await page.locator("#account-status").evaluate((node) => { + const box = node.getBoundingClientRect(); + return box.top >= 0 && box.bottom <= window.innerHeight; + }), + true, + ); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal(logins, 0); + assert.equal(await page.isDisabled("#start"), false); + }, + { viewport: { width: 375, height: 700 } }, +); + +// Until the session answers, the guard above cannot tell a server that requires +// sign-in from one that admits guests, so Start waits rather than guessing. +lobbyTest( + "Start waits for the session when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let releaseSession; + holdSession = new Promise((resolve) => (releaseSession = resolve)); + try { + const sessionRequest = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "load" }); + await sessionRequest; + await page.click("details.problem-picker summary"); + await page.click(`[data-problem="${MEDIUM[0]}"]`); + assert.equal(await page.isDisabled("#start"), true); + await page.evaluate(() => { + document.querySelector("#start").dispatchEvent(new MouseEvent("click")); + }); + await normalPageshow(page); + assert.equal(new URL(page.url()).pathname, "/"); + } finally { + releaseSession(); + } + await awaitReady(page); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Use GitHub to start", + ); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// A failed check cannot be waved through here as it is where storage works: a +// server that requires sign-in would send the candidate straight back. +lobbyTest( + "Start offers the account retry when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + failing.add("/api/session"); + await lobby(page); + assert.equal( + await page.locator("#account-status").textContent(), + "Could not check your account. Retry the account check.", + ); + // Nobody signed in, so there is nothing to sign out of. + assert.equal(await page.locator("#logout").isHidden(), true); + assert.equal(await page.isDisabled("#start"), true); + await page.evaluate(() => { + document.querySelector("#start").dispatchEvent(new MouseEvent("click")); + }); + await normalPageshow(page); + assert.equal(new URL(page.url()).pathname, "/"); + + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Use GitHub to start", + ); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// No login lies behind this retry, so its answer must not read as a failed one. +lobbyTest( + "a retry on a guest server does not ask for a sign-in that never happened", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: true, + }; + let logins = 0; + page.on("request", (request) => { + if (new URL(request.url()).pathname === "/api/login") logins++; + }); + failing.add("/api/session"); + await lobby(page); + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.doesNotMatch( + await page.locator("#account-status").textContent(), + /Try signing in again/, + ); + assert.equal(logins, 0); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// The answer that matters most: a retry that finds the candidate signed in has +// to clear the failed check, or Start and every delete stay down for good. +lobbyTest( + "a signed-in answer to the retry releases Start when storage is blocked", + async (page) => { + await blockSessionStorage(page); + failing.add("/api/session"); + await lobby(page); + assert.equal(await page.isDisabled("#start"), true); + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.equal(await page.isDisabled("#start"), false); + assert.match( + await page.locator("#account-status").textContent(), + /Signed in as candidate/, + ); + }, +); + +// Coming back from a Start that left: the retry state must not keep showing the +// label of a start that is no longer on its way. +lobbyTest( + "a restored lobby drops Starting when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { signedIn: false, loginRequired: false }; + await lobby(page); + // Put there rather than pressed: a navigation held open stalls every later + // step of the test, and only what the restore does with the label is at stake. + await page.evaluate(() => { + document.querySelector("#start").textContent = "Starting..."; + }); + failing.add("/api/session"); + await restore(page); + assert.equal( + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("Could not check your account"), + ), + true, + ); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Start interview", + ); + }, +); + +// A login the server accepted stays known as recorded: only the account check +// after it failed, and the candidate should not be told the sign-in is in doubt. +lobbyTest( + "a recorded login stays recorded when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + await page.route("**/api/login", (route) => { + failing.add("/api/session"); + return route.fulfill({ json: { ok: true } }); + }); + await lobby(page); + await page.fill("#github-login", "candidate"); + await page.click("#login-link"); + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("account could not be refreshed"), + ); + assert.match( + await page.locator("#account-status").textContent(), + /GitHub username recorded, but the account could not be refreshed/, + ); + }, +); + +// The hold is for blocked storage alone: elsewhere Practice may still start on +// a manual pick before the account answers. +lobbyTest( + "Start does not wait for the session when storage works", + async (page) => { + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let releaseSession; + holdSession = new Promise((resolve) => (releaseSession = resolve)); + try { + const sessionRequest = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "load" }); + await sessionRequest; + await page.click("details.problem-picker summary"); + await page.click(`[data-problem="${MEDIUM[0]}"]`); + assert.equal(await page.isDisabled("#start"), false); + } finally { + releaseSession(); + } + }, +); + +// A signed-out answer is an answer: where guests are admitted, blocked storage +// is no reason to offer a retry, and Start goes on to the interview. +lobbyTest( + "a guest starts when the session answers and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { signedIn: false, loginRequired: false }; + await lobby(page); + assert.equal(await page.isDisabled("#start"), false); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + }, +); + +// Strict cannot run without the tab's storage, so it stays on the lobby. What is +// pinned is that the reason is the one selection.js wrote for the candidate, not +// the browser's own exception text. +lobbyTest("Strict names session storage when it is blocked", async (page) => { + await blockSessionStorage(page); + await lobby(page); + await page.selectOption("#selection-mode", "strict"); + await page.click("#start"); + await page.waitForFunction( + () => document.querySelector("#grounding-error").textContent !== "", + ); + assert.match( + await page.locator("#grounding-error").textContent(), + /Strict Interview needs session storage/, + ); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal(await page.isDisabled("#start"), false); +}); + lobbyTest( "editor options default to all five features enabled", async (page) => { diff --git a/web/app.js b/web/app.js index f7ce345a..1b0467d1 100644 --- a/web/app.js +++ b/web/app.js @@ -115,6 +115,9 @@ let loginPending = false; // Successful and uncertain POSTs need an account check before another login. // Null means no check is owed; otherwise this is "recorded" or "unknown". let loginResult = null; +// A session check that failed where storage is blocked. It holds Start like the +// above and is retried the same way, but no POST lies behind it. +let accountCheckFailed = false; let deletingReports = false; /// The longest interview this deployment can record, which only the server /// knows. `Infinity` until `/api/session` answers: the duration row is live @@ -766,6 +769,7 @@ start.addEventListener("click", async () => { (!historyReady || accountHistory !== true)) || starting || accountUpdatePending() || + sessionUnknownWithoutStorage() || deletingReports ) return; @@ -819,9 +823,20 @@ start.addEventListener("click", async () => { return; } + nodes.groundingError.textContent = ""; + // Assumes a block on storage also refuses the session cookie, where signing in + // here would reach the interview signed out and come straight back. A block on + // storage alone can still sign in through Use GitHub, which stays ungated. + if (signInFirst && !storageArea("sessionStorage")) { + nodes.accountStatus.textContent = + "Signing in needs site data, which this browser is blocking for this site. Allow site data for this site, then reload this page."; + // On a narrow page Start sits far below the account row, out of its view. + nodes.githubLogin.focus(); + return; + } + starting = true; syncLobbyControls(); - nodes.groundingError.textContent = ""; if (signInFirst) { start.textContent = "Recording GitHub..."; if (!(await recordGitHubLogin())) { @@ -840,9 +855,10 @@ start.addEventListener("click", async () => { start.textContent = "Starting..."; if (!nodes.groundingConsent.checked) packet = null; try { - if (strict) resetAssignments(sessionStorage); - storeGroundingPacket(sessionStorage, packet); - storeSharedFocus(sessionStorage, focus?.weakness ?? null); + const tabStorage = storageArea("sessionStorage"); + if (strict) resetAssignments(tabStorage); + storeGroundingPacket(tabStorage, packet); + storeSharedFocus(tabStorage, focus?.weakness ?? null); } catch (error) { nodes.groundingError.textContent = error.message; starting = false; @@ -921,7 +937,7 @@ function clearGrounding() { nodes.groundingError.textContent = ""; nodes.groundingChoices.replaceChildren(); try { - storeGroundingPacket(sessionStorage, null); + storeGroundingPacket(storageArea("sessionStorage"), null); } catch { /* clearing is best effort */ } @@ -970,7 +986,7 @@ nodes.loginLink.addEventListener("click", async () => { syncLobbyControls(); try { // Retrying an account check must not create another recorded session. - if (!loginResult) { + if (!loginResult && !accountCheckFailed) { if (!(await recordGitHubLogin())) return; loginResult = "recorded"; } @@ -993,6 +1009,7 @@ function syncLobbyControls() { (!historyReady || accountHistory !== true)) || starting || accountBusy || + sessionUnknownWithoutStorage() || deletingReports; selection.disabled = starting; nodes.loginLink.disabled = @@ -1020,7 +1037,20 @@ function syncLobbyControls() { } function accountUpdatePending() { - return loginPending || loginResult !== null || historyRefreshing; + return ( + loginPending || + loginResult !== null || + accountCheckFailed || + historyRefreshing + ); +} + +/// Practice may start before the account answers, except where storage is +/// blocked: the answer then decides whether Start can sign in at all. +function sessionUnknownWithoutStorage() { + return ( + !historyReady && accountHistory === null && !storageArea("sessionStorage") + ); } /// One rule for the bulk delete and every row's: until the lobby knows whose @@ -1119,6 +1149,7 @@ async function loadAccount(generation) { throw new Error("Invalid account response"); } accountHistory = false; + accountCheckFailed = false; const peerLink = document.getElementById("peer-practice"); if (peerLink) peerLink.hidden = !( @@ -1176,7 +1207,14 @@ async function loadAccount(generation) { } } if (generation !== historyLoad) return; - if (loginResult) { + // Not waved through as the catch above says: with storage blocked, a server + // that requires sign-in would send the candidate straight back. Retry is GET. + if (accountHistory === null && !storageArea("sessionStorage")) { + accountCheckFailed = loginResult === null; + // A restored page may still be labelled for the start that left it. + setStartGate(signInFirst); + } + if (loginResult || accountCheckFailed) { showAccountCheckError(); return; } @@ -1189,14 +1227,15 @@ async function loadAccount(generation) { } function showAccountCheckError() { - nodes.accountStatus.textContent = - loginResult === "recorded" + nodes.accountStatus.textContent = accountCheckFailed + ? "Could not check your account. Retry the account check." + : loginResult === "recorded" ? "GitHub username recorded, but the account could not be refreshed." : "Could not confirm the sign-in result. Retry the account check."; nodes.githubLogin.hidden = true; nodes.loginLink.hidden = false; nodes.loginLink.textContent = "Retry account check"; - nodes.logout.hidden = false; + nodes.logout.hidden = accountCheckFailed; showProgressError( "Could not load account progress. Retry the account check.", ); diff --git a/web/audio-output.js b/web/audio-output.js index 6bbf644c..706ce691 100644 --- a/web/audio-output.js +++ b/web/audio-output.js @@ -163,8 +163,8 @@ function showOutputNote(text) { /// there, touching either global throws rather than returning a storage that /// refuses. The helpers below default through this rather than naming the /// global, because a default parameter is evaluated before their `try`, and -/// every reader of the result already treats a failing call as "nothing -/// stored", which `null` is. +/// they treat a failing call as "nothing stored", which `null` is. A caller +/// that must tell a failure from an empty store has to check for null itself. export function storageArea(name) { try { return globalThis[name]; diff --git a/web/history.js b/web/history.js index 6f9c6532..062bb84f 100644 --- a/web/history.js +++ b/web/history.js @@ -1,3 +1,5 @@ +import { storageArea } from "./audio-output.js"; + export const historyKey = "codetrial_history"; export const reviewHistoryKey = "codetrial_review_history"; @@ -14,6 +16,16 @@ const REVIEW_HISTORY_BYTES = 164 * 1024; /// A deadline turns both into the failure they already know how to report. const requestTimeoutMs = 10_000; +/// The store a caller passed, or this device's, and a throw where neither can be +/// reached. `storageArea` answers null there, and `storedList` reads a null store +/// as "no list stored", so carrying it on would let `deleteReport` claim a removal +/// it never ran. The throw reaches each caller's catch, as the bare global's did. +function deviceStorage(storage) { + const store = storage || storageArea("localStorage"); + if (!store) throw new Error("local storage is unavailable"); + return store; +} + /// Always a list. The key sits in the origin's local storage, which an older /// build, another tab, or anyone with devtools open can write, so what it holds /// is input rather than something this module chose. Unparseable JSON was @@ -34,7 +46,7 @@ export function readLocalHistory(storage) { /// not a list, and the shape check already turns that into one. function storedList(key, storage) { try { - const stored = JSON.parse((storage || localStorage).getItem(key)); + const stored = JSON.parse(deviceStorage(storage).getItem(key)); return Array.isArray(stored) ? stored : null; } catch { return null; @@ -43,7 +55,7 @@ function storedList(key, storage) { export function readReviewHistory(storage) { try { - storage ||= localStorage; + storage = deviceStorage(storage); const stored = storedList(reviewHistoryKey, storage); if (stored) return stored; const { retained, json } = boundedReviews(readLocalHistory(storage)); @@ -73,7 +85,7 @@ export function readReviewHistory(storage) { /// deduplicated within a store quietly merged them. export function readDeviceHistory(storage) { try { - storage ||= localStorage; + storage = deviceStorage(storage); const history = readLocalHistory(storage); const reviews = readReviewHistory(storage); // Mutates both arrays, and writes both stores when it has anything to say. @@ -194,7 +206,7 @@ function legacyKey(entry) { /// from the history when it is first read, after this has run. export function renameLocalHistory(pages, storage, markUnmapped = false) { try { - storage ||= localStorage; + storage = deviceStorage(storage); const history = renamedEntries( readLocalHistory(storage), pages, @@ -301,7 +313,7 @@ export async function clearReportHistory({ return "failed"; } try { - storage ||= localStorage; + storage = deviceStorage(storage); storage.removeItem(historyKey); storage.removeItem(reviewHistoryKey); return "cleared"; @@ -348,7 +360,7 @@ export async function deleteReport( } let removed = fromAccount; try { - storage ||= localStorage; + storage = deviceStorage(storage); // A missing review store is left missing: it is rebuilt from the short // history on its first read, and that no longer holds the report. for (const key of [historyKey, reviewHistoryKey]) { @@ -374,7 +386,7 @@ export async function deleteReport( function saveLocalReport(entry, storage) { try { - storage ||= localStorage; + storage = deviceStorage(storage); const previous = readLocalHistory(storage); // The short history first, and before the review store is even read. // Reading it can rebuild it, and that write is the 164 KB one: spending the