Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,30 @@ 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 web/audio-output.js.";
const STORAGE_ACCESS = {
"no-restricted-globals": [
"error",
...["localStorage", "sessionStorage"].map((name) => ({
name,
message: STORAGE_MESSAGE,
})),
],
"no-restricted-properties": [
"error",
...["localStorage", "sessionStorage"].map((property) => ({
property,
message: STORAGE_MESSAGE,
})),
],
Comment thread
sidney-Hung marked this conversation as resolved.
Resolved
};

export default [
{
// `externals/` holds unrelated checkouts, is untracked, and is excluded in
Expand All @@ -84,6 +108,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"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A file-wide ignores also lets a sixth storage reference into web/peer-lobby.js without a warning while the fix is pending. Putting // eslint-disable-next-line no-restricted-globals on each of the five existing references keeps the rule active for new code, and reportUnusedDisableDirectives: "error" makes each fix delete its own directive, so the exemption is gone once the last one is fixed. On the follow-up issue the description offers: yes, please open it. The module-scope setItem on an invite link is the same bug as #600 on another page.

rules: STORAGE_ACCESS,
},
{
// Node: the browser tests and the generator scripts.
//
Expand Down
40 changes: 38 additions & 2 deletions tests/browser/account.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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\)/);
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -1217,6 +1220,39 @@ 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.
// Guards the null `storageArea` answers: passed through, `storedList` reads it
// as an empty store and the delete answers "missing".
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.
Expand Down
50 changes: 37 additions & 13 deletions tests/browser/lobby-render.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -29,28 +29,52 @@ import { installDocument } from "./dom.js";
const markup = read("web/index.html");
const dom = installDocument(markup);

// Site data blocked, said here rather than left to the runtime: from Node 25
// `sessionStorage` is a working store by default and `localStorage` is one
// once `--localstorage-file` names a file (an empty object without it), while
// plain `node` before 25 has neither. What the page renders would depend on
// how Node runs it, so both throw on access, as a browser with site data
// blocked does.
for (const name of ["localStorage", "sessionStorage"]) {
Object.defineProperty(globalThis, name, {
configurable: true,
get() {
throw new DOMException("blocked", "SecurityError");
},
});
}

// Imported once, at the top, because ESM caches by URL and the page does its
// 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",
/// Neither is contrived: `installDocument` refuses the fetch the page starts at
/// import, so the session lookup fails, and the storage block above decides
/// the rest. The blocked `sessionStorage` is what turns that failed lookup into
/// the account retry: `account-status`, `login-link`, `start` and
/// `progress-summary` all follow from it. The blocked `localStorage` is what
/// makes the practice list unreadable, which is the only reason
/// `practice-list-status` ends by saying browser storage is unavailable. No
/// expectation here needs either store to work. 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 = {
Comment thread
sidney-Hung marked this conversation as resolved.
Resolved
"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}`);
}
});
Expand All @@ -64,7 +88,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,
Expand Down
Loading
Loading