fix: settle a site approval on the port that carries its teardown (closes #275)

Approve and window.close() left the popup on the next line, and the decision
and the disconnect the close caused travelled independent channels with nothing
ordering them. The disconnect handler settled a pending site approval as a
rejection, so whichever landed first decided the outcome. Driven in a tab the
teardown won every time: the user allowed the connection and the dApp was told
they had refused.

The decision now goes out on the approval port the popup already opens, which
is the same port the close disconnects. One channel is ordered -- a message
posted on a port is delivered before that port's own disconnect -- so the
approval is settled before the teardown is even seen, and the disconnect then
finds nothing pending to reject. Nothing waits, nothing is timed, and the popup
closes exactly as immediately as before.

windows.onRemoved no longer decides a site approval whose port is connected
either. In the fallback-window shape that event races the decision on a channel
of its own, which is the same defect one level over; the port disconnect says
the same thing in a defined order, so it is left to say it. A window that
closes before its popup ever connected has nothing else to speak for it and is
still rejected there, so no dApp is left waiting on a window that is gone.

Rejecting reports a rejection, and so does closing without deciding, in both
shapes. AUTISTMASK_APPROVAL_RESPONSE is gone; the port name carries the
approval id, so the popup no longer names one, and the sender check the message
carried moved to the port.

tests/backgroundApproval.test.js drives decide-then-disconnect with nothing
awaited in between, in the toolbar-popup shape that production uses and in the
fallback-window shape, and asserts every close-without-deciding path still
rejects. tests/e2e/run.js drops the deferred-window.close() accommodation it
carried for this bug, so the two site-prompt tests now drive the shipped
decide-then-close in a real Chromium.

README.md

@@ -165,23 +165,6 @@ fixtures in `tests/e2e/network.js`, so the run is deterministic and fully
 offline; unrecognised outbound requests are reported as failures rather than
 silently allowed.
 
-It also covers the **Settings screen**, which holds the densest run of element
-id lookups in the codebase and where one wrong id leaves the whole popup blank
-rather than only degrading Settings: that the screen renders populated — the
-About well and the wallet list are read back, so a `show()` that stopped early
-is caught rather than merely a view that failed to appear — that the four Token
-Spam Protection controls are real checkboxes defaulted on, and that the theme
-and network selectors offer exactly the choices `src/shared/networks.js` and
-`index.html` define. What the selectors persist is then driven through the UI to
-`dark` and `sepolia` and reasserted after a popup reopen before being restored
-the same way; neither value is its selector's first `<option>`, so neither can
-be read back from the markup with no JavaScript having run. One spam filter is
-likewise toggled off and back on across a reopen each way, which exercises the
-change handler, `saveState()`, `loadState()` and the assignment `init()` makes.
-Each group of these assertions records a coverage key and a final case demands
-the exact set, so a section that silently stopped running reddens the suite
-instead of quietly shrinking it.
-
 It also covers the confirmation screen, for both a native ETH send and an ERC-20
 send: Send disabled while the fee estimate is in flight, enabled once it lands,
 the fee block quoting the expected cost and the reserve separately, the distinct
@@ -417,24 +400,6 @@ the one-time pull of the pinned ~800MB Playwright layer, and well under a minute
 once that layer is cached. `e2e-firefox` about 1m05s cold, and it caches its
 Firefox and geckodriver downloads the same way.
 
-### Element id guard (part of `make check`)
-
-`tests/popupElementIds.test.js` asserts statically that every element id the
-popup looks up — `$("...")`, `document.getElementById("...")`,
-`showError()`/`hideError()`, and the `view-<name>` a literal `showView("...")`
-resolves to — exists in `src/popup/index.html`, and that `index.html` defines no
-id twice. A wrong id is valid JavaScript naming a defined function, so neither
-jest (node environment, no DOM) nor a linter objects to it; at runtime `$()`
-returns `null` and the next property access throws, which inside a view's
-`init()` aborts the rest of `src/popup/index.js` `init()` and leaves the popup
-blank.
-
-It runs with no browser, so unlike the e2e suites it fits inside `make check`,
-and it covers every view rather than the ones some test happens to open. It only
-sees literal arguments: a call like `$(containerId)` is invisible to it, and a
-lookup naming the wrong existing element is valid by construction. Both of those
-are the browser suites' job.
-
 ## Rationale
 
 Common popular EVM wallets have become bloated with swap UIs, portfolio

TODO.md

@@ -44,34 +44,6 @@ but the review is broader than any of them.
 
 # Completed Steps
 
-- 2026-08-17: The Settings screen is driven in a browser, and every element id
-  the popup looks up is checked statically. Nothing exercised Settings in the
-  e2e suite, and jest runs with no DOM, so the densest run of `$("...")` lookups
-  in the codebase was unverified at runtime. Seven new cases in
-  `tests/e2e/run.js` reach Settings, assert the About well and the wallet list
-  were actually written, assert the four Token Spam Protection checkboxes are
-  real checkboxes defaulted on, and assert the theme and network selectors offer
-  the choices `src/shared/networks.js` and `index.html` define. The selectors
-  are then driven to `dark` and `sepolia` — neither is the first `<option>`, so
-  neither can be read back from the markup with no JavaScript involved — and
-  reasserted after a popup reopen before being restored the same way, and one
-  spam filter is toggled off and back on across a reopen each way. Those round
-  trips run the change handler, `saveState()`, `loadState()` and the
-  `init()`/`show()` assignments rather than just looking at the screen. `show()`
-  no longer guards its `settings-network` lookup with `if (networkSelect)`: a
-  missing element must fail loudly, which is the whole failure mode this unit
-  exists to catch. Each group records a coverage key and a final case demands
-  the exact set, so a shortened or skipped section reddens the run instead of
-  shrinking it. `tests/popupElementIds.test.js` is the general half and runs in
-  `make check` with no browser: every literal id reached through `$()`,
-  `document.getElementById()`, `showError()`/`hideError()` and `showView()` must
-  exist in `src/popup/index.html`, which no id in `index.html` may define twice.
-  Demonstrated on four deliberate breaks — a typo'd id (both halves red), a
-  handler bound to the wrong but existing element (only the functional e2e case
-  red), a typo in a view no browser suite opens (only the static guard red), and
-  the deletion of both persisted-value assignments in `settings.js` (only the
-  selector round-trip case red)
-  ([#229](https://git.eeqj.de/sneak/AutistMask/issues/229)).
 - 2026-08-17: One wording for an empty password field on every screen that asks
   for one. The private key export screen said "Password is required." where the
   other five say "Please enter your password.", the same one-condition-two-
@@ -204,27 +176,6 @@ but the review is broader than any of them.
   under load, filed as [#287](https://git.eeqj.de/sneak/AutistMask/issues/287)
   rather than papered over
   ([#259](https://git.eeqj.de/sneak/AutistMask/issues/259)).
-- 2026-08-14: A background message handler that throws now rejects the page
-  instead of hanging it. `handleRpc(...).then(sendResponse)` had no `.catch()`,
-  and `sendResponse` is the only thing that settles the dApp's
-  `window.ethereum.request()` promise — so any throw inside `handleRpc` left
-  that promise pending forever, with no error and no timeout, indistinguishable
-  from a slow wallet. It now answers `{ code: -32603, message }` (the JSON-RPC
-  internal error EIP-1474 defines and EIP-1193 defers to; no EIP-1193 4xxx code
-  describes "the wallet broke" and none was invented) and logs the method and
-  the throw to the background console rather than swallowing them. The two async
-  IIFEs behind `AUTISTMASK_TX_RESPONSE` and `AUTISTMASK_SIGN_RESPONSE` were the
-  same shape one level down — every statement inside a `try`, but a throw out of
-  a `catch` block escaping unhandled — and each got a last-resort `.catch()`
-  settling the approval through `settleApproval()` and answering the popup. The
-  transaction one tracks which phase it escaped from and reports that, so an
-  escape before `broadcastTransaction()` says the request is gone rather than
-  that it may still have reached the network. Every other handler on the path is
-  synchronous. All four are driven by real failures — a rejecting storage read,
-  and a failure classifier that throws while classifying a genuine verification
-  or broadcast failure — and were demonstrated failing first, the RPC one with
-  `sendResponse` at zero calls
-  ([#280](https://git.eeqj.de/sneak/AutistMask/issues/280)).
 - 2026-08-14: Approving a site connection is no longer a race against the popup
   closing. The decision now rides the approval port the popup already holds,
   which is the same channel the close disconnects, so it is delivered ahead of

src/background/index.js

@@ -170,16 +170,6 @@ function approvedNonce(approvedTx) {
     }
 }
 
-// What the page is told when a request failed in a way the wallet has no
-// specific answer for. -32603 is the JSON-RPC internal error EIP-1474 defines
-// and EIP-1193 defers to for RPC-layer failures; no EIP-1193 4xxx code
-// describes "the wallet broke", and one is not invented here. The cause is
-// logged rather than put in the message: the page gets a stable sentence, the
-// background console gets the throw.
-const INTERNAL_ERROR_CODE = -32603;
-const INTERNAL_ERROR_MESSAGE =
-    "AutistMask could not complete this request because of an internal error.";
-
 async function getState() {
     const result = await storageGet("autistmask");
     return (
@@ -1153,26 +1143,9 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                 // keep fallback
             }
         }
-        handleRpc(msg.method, msg.params, trustedOrigin)
+        handleRpc(msg.method, msg.params, trustedOrigin).then((response) => {
-            .then((response) => {
+            sendResponse(response);
-                sendResponse(response);
+        });
-            })
-            .catch((err) => {
-                // Without this the page's window.ethereum.request() promise
-                // stays pending forever: no response is sent, the content
-                // script posts nothing back, and the dApp cannot tell the
-                // failure from a slow wallet. handleRpc does real work —
-                // state loads, provider calls, transaction population — so
-                // "it does not throw today" is not a property anyone is
-                // maintaining.
-                log.errorf("RPC request failed:", msg.method, err);
-                sendResponse({
-                    error: {
-                        code: INTERNAL_ERROR_CODE,
-                        message: INTERNAL_ERROR_MESSAGE,
-                    },
-                });
-            });
         return true;
     }
 
@@ -1268,10 +1241,6 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
             return false;
         }
 
-        // Which phase the last-resort .catch() below reports. Everything up to
-        // the broadcastTransaction() call provably never reached the network,
-        // so an escape from there must not tell the user it might have.
-        let lastResortStage = TX_STAGE_VERIFY;
         (async () => {
             // The chain this attempt is on, read once. Verification below
             // refuses an artifact signed for any other chain, and the nonce
@@ -1355,7 +1324,6 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
 
             try {
                 const provider = getProvider(state.rpcUrl);
-                lastResortStage = TX_STAGE_BROADCAST;
                 const tx = await provider.broadcastTransaction(msg.rawSignedTx);
                 if (nonce !== null) spent.add(nonce);
                 settleApproval(
@@ -1387,28 +1355,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                     stage: outcome.stage,
                 });
             }
-        })().catch((e) => {
+        })();
-            // Every statement above is inside a try, but a throw from one of
-            // the catch blocks escapes as an unhandled rejection and neither
-            // the popup nor the page is ever answered. Settle both, through
-            // the same chokepoint as every other retirement.
-            log.errorf("transaction approval response failed:", e);
-            settleApproval(
-                msg.id,
-                {
-                    error: {
-                        code: INTERNAL_ERROR_CODE,
-                        message: INTERNAL_ERROR_MESSAGE,
-                    },
-                },
-                { holdsClaim: true },
-            );
-            sendResponse({
-                error: INTERNAL_ERROR_MESSAGE,
-                retryable: false,
-                stage: lastResortStage,
-            });
-        });
         return true;
     }
 
@@ -1492,25 +1439,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                 }
                 sendResponse({ error: errMsg, retryable });
             }
-        })().catch((e) => {
+        })();
-            // Same shape as the transaction path: a throw out of the catch
-            // block above would leave the popup and the page both waiting.
-            log.errorf("sign approval response failed:", e);
-            settleApproval(
-                msg.id,
-                {
-                    error: {
-                        code: INTERNAL_ERROR_CODE,
-                        message: INTERNAL_ERROR_MESSAGE,
-                    },
-                },
-                { holdsClaim: true },
-            );
-            sendResponse({
-                error: INTERNAL_ERROR_MESSAGE,
-                retryable: false,
-            });
-        });
         return true;
     }
 

src/popup/views/settings.js

@@ -167,7 +167,10 @@ function renderWalletListSettings() {
 function show() {
     $("settings-rpc").value = state.rpcUrl;
     $("settings-blockscout").value = state.blockscoutUrl;
-    $("settings-network").value = state.networkId;
+    const networkSelect = $("settings-network");
+    if (networkSelect) {
+        networkSelect.value = state.networkId;
+    }
     renderTrackedTokens();
     renderSiteLists();
     renderWalletListSettings();
@@ -279,13 +282,15 @@ function init(ctx) {
     });
 
     const networkSelect = $("settings-network");
-    networkSelect.addEventListener("change", async () => {
+    if (networkSelect) {
-        const newId = networkSelect.value;
+        networkSelect.addEventListener("change", async () => {
-        const net = await onChainSwitch(newId);
+            const newId = networkSelect.value;
-        $("settings-rpc").value = state.rpcUrl;
+            const net = await onChainSwitch(newId);
-        $("settings-blockscout").value = state.blockscoutUrl;
+            $("settings-rpc").value = state.rpcUrl;
-        showFlash("Switched to " + net.name + ".");
+            $("settings-blockscout").value = state.blockscoutUrl;
-    });
+            showFlash("Switched to " + net.name + ".");
+        });
+    }
 
     $("settings-show-zero-balances").checked = state.showZeroBalanceTokens;
     $("settings-show-zero-balances").addEventListener("change", async () => {

tests/backgroundApproval.test.js

@@ -20,11 +20,6 @@
 
 const { Network, Wallet } = require("ethers");
 
-// The real formatter the approval screen renders failures through. Bound here,
-// before any jest.doMock() of the module, so the copy assertions below check
-// what the user is actually shown.
-const { describeSigningFailure } = require("../src/shared/approvalVerify");
-
 const SIGNER_KEY =
     "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
 const OTHER_KEY =
@@ -167,14 +162,6 @@ function loadBackground(options) {
         ensureRecurringAlarms: jest.fn(async () => {}),
         registerAlarmHandlers: jest.fn(),
     }));
-    // The real verification module, except where a test replaces one export
-    // with a throw to drive the handler's own error handling into failing.
-    if (opts.approvalVerify) {
-        jest.doMock("../src/shared/approvalVerify", () => ({
-            ...jest.requireActual("../src/shared/approvalVerify"),
-            ...opts.approvalVerify,
-        }));
-    }
 
     const persisted = {
         wallets: [
@@ -199,10 +186,7 @@ function loadBackground(options) {
     global.chrome = {
         storage: {
             local: {
-                get: jest.fn(
+                get: jest.fn(async () => ({ autistmask: persisted })),
-                    opts.storageGet ||
-                        (async () => ({ autistmask: persisted })),
-                ),
                 set: jest.fn(async () => {}),
             },
         },
@@ -428,15 +412,6 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
-// settle() only drains microtasks. A handler whose last-resort .catch() has to
-// run after a macrotask boundary needs those turns too, so the assertion that
-// the page WAS answered is what reports a regression rather than a timeout.
-async function settleIncludingRejections() {
-    await settle();
-    await new Promise((resolve) => setImmediate(resolve));
-    await new Promise((resolve) => setImmediate(resolve));
-}
-
 afterEach(() => {
     delete global.chrome;
     jest.resetModules();
@@ -1503,200 +1478,6 @@ describe("a claimed approval outlives every other retirement path", () => {
     });
 });
 
-// A handler that throws must still answer. `sendResponse` is the only thing
-// that settles the page's window.ethereum.request() promise, so a throw that
-// escapes a handler leaves that promise pending forever — no error, no
-// timeout, indistinguishable from a slow wallet. Each case below drives a real
-// throw out of a handler rather than asserting the catch block exists.
-describe("a handler that throws still settles the page", () => {
-    const INTERNAL_ERROR = {
-        code: -32603,
-        message:
-            "AutistMask could not complete this request because of an internal error.",
-    };
-
-    let errorLog;
-    beforeEach(() => {
-        errorLog = jest.spyOn(console, "error").mockImplementation(() => {});
-    });
-    afterEach(() => {
-        errorLog.mockRestore();
-    });
-
-    // getState() awaits extension storage unguarded, and every read path in
-    // handleRpc goes through it. A storage read that rejects is the whole
-    // failure — no hook in the handler itself.
-    test("a rejected handleRpc rejects the page instead of hanging it", async () => {
-        const bg = loadBackground({
-            storageGet: async () => {
-                throw new Error("storage unavailable");
-            },
-        });
-
-        const answer = bg.send(
-            { type: "AUTISTMASK_RPC", method: "eth_accounts", params: [] },
-            { origin: ORIGIN },
-        );
-        await settleIncludingRejections();
-
-        // The channel is held open for the async answer, and the answer
-        // arrives.
-        expect(answer.kept).toBe(true);
-        expect(answer.sendResponse).toHaveBeenCalledWith({
-            error: INTERNAL_ERROR,
-        });
-        // Not swallowed: the throw is on the background console, which is how
-        // this class gets caught in future.
-        expect(errorLog).toHaveBeenCalledWith(
-            "[AutistMask]",
-            "RPC request failed:",
-            "eth_accounts",
-            expect.objectContaining({ message: "storage unavailable" }),
-        );
-    });
-
-    // The transaction response handler wraps every statement in a try, so what
-    // escapes it is a throw from inside one of its catch blocks. Here the
-    // failure classifier itself throws while classifying a real verification
-    // failure — the approval is left claimed, so nothing else can settle it.
-    // The escape happens before broadcastTransaction() is reached, so the
-    // reported stage must be the one that says the transaction is gone.
-    test("a throw while verifying a transaction settles both the page and the popup", async () => {
-        const bg = loadBackground({
-            approvalVerify: {
-                describeTxFailure: () => {
-                    throw new Error("classifier broke");
-                },
-            },
-        });
-        const pending = bg.requestTx();
-        await settle();
-        const id = pending.id();
-
-        // A real verification failure: the artifact is signed at a nonce the
-        // approval never displayed.
-        const answer = bg.send(
-            {
-                type: "AUTISTMASK_TX_RESPONSE",
-                id,
-                approved: true,
-                rawSignedTx: await signedAtNonce(NONCE + 1),
-            },
-            { url: bg.fromPopup.url },
-        );
-        await settleIncludingRejections();
-
-        expect(bg.broadcastTransaction).not.toHaveBeenCalled();
-        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
-        expect(answer.sendResponse).toHaveBeenCalledWith({
-            error: INTERNAL_ERROR.message,
-            retryable: false,
-            // Nothing was broadcast, so the popup must say the request is gone
-            // rather than that it may still have reached the network.
-            stage: "verify",
-        });
-        expect(errorLog).toHaveBeenCalledWith(
-            "[AutistMask]",
-            "transaction approval response failed:",
-            expect.objectContaining({ message: "classifier broke" }),
-        );
-        // The copy the user actually reads, from the popup's own formatter.
-        expect(
-            describeSigningFailure(answer.sendResponse.mock.calls[0][0], "")
-                .message,
-        ).toBe(
-            INTERNAL_ERROR.message +
-                " This request can no longer be signed." +
-                " Please start it again from the site.",
-        );
-    });
-
-    // The other side of the same local: once broadcastTransaction() has been
-    // entered the wallet genuinely cannot tell whether the node took the
-    // transaction, and the copy that warns about a second send is correct.
-    test("a throw while handling a failed broadcast reports the broadcast stage", async () => {
-        const bg = loadBackground({
-            approvalVerify: {
-                describeTxFailure: () => {
-                    throw new Error("classifier broke");
-                },
-            },
-        });
-        bg.broadcastTransaction.mockRejectedValue(new Error("node refused"));
-        const pending = bg.requestTx();
-        await settle();
-        const id = pending.id();
-
-        // The approved artifact, so verification passes and the failure
-        // happens at the broadcast.
-        const answer = bg.send(
-            {
-                type: "AUTISTMASK_TX_RESPONSE",
-                id,
-                approved: true,
-                rawSignedTx: await signedAtNonce(NONCE),
-            },
-            { url: bg.fromPopup.url },
-        );
-        await settleIncludingRejections();
-
-        expect(bg.broadcastTransaction).toHaveBeenCalled();
-        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
-        expect(answer.sendResponse).toHaveBeenCalledWith({
-            error: INTERNAL_ERROR.message,
-            retryable: false,
-            stage: "broadcast",
-        });
-        expect(
-            describeSigningFailure(answer.sendResponse.mock.calls[0][0], "")
-                .message,
-        ).toBe(
-            INTERNAL_ERROR.message +
-                " The transaction may still have reached the network." +
-                " Check the account before sending it again.",
-        );
-    });
-
-    test("a throw while handling a failed signature settles both the page and the popup", async () => {
-        const bg = loadBackground({
-            approvalVerify: {
-                failureIsRetryable: () => {
-                    throw new Error("classifier broke");
-                },
-            },
-        });
-        const pending = bg.requestSign();
-        await settle();
-
-        // A real verification failure: the active address moved after the
-        // approval was raised.
-        bg.setActiveAddress(other.address);
-        const answer = bg.send(
-            {
-                type: "AUTISTMASK_SIGN_RESPONSE",
-                id: pending.id(),
-                approved: true,
-                signature: await signer.signMessage(
-                    Buffer.from(MESSAGE.slice(2), "hex"),
-                ),
-            },
-            { url: bg.fromPopup.url },
-        );
-        await settleIncludingRejections();
-
-        expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
-        expect(answer.sendResponse).toHaveBeenCalledWith({
-            error: INTERNAL_ERROR.message,
-            retryable: false,
-        });
-        expect(errorLog).toHaveBeenCalledWith(
-            "[AutistMask]",
-            "sign approval response failed:",
-            expect.objectContaining({ message: "classifier broke" }),
-        );
-    });
-});
-
 describe("popup-only messages", () => {
     test("a page sender cannot answer an approval", async () => {
         const bg = loadBackground();

tests/e2e/run.js

@@ -42,7 +42,6 @@ const {
     STUB_TX_HASH,
 } = require("./network");
 const { DUST_THRESHOLD_MESSAGE } = require("../../src/popup/dustThreshold");
-const { NETWORKS } = require("../../src/shared/networks");
 
 const TEST_TIMEOUT_MS = 120000;
 
@@ -945,328 +944,6 @@ test("confirming removes the address and returns Home (#162)", async (env) => {
     );
 });
 
-// ------------------------------------------------- Settings screen (#229)
-
-// Settings holds the densest run of $("...") lookups in the codebase, and
-// until this section nothing drove it in a browser. One wrong id makes
-// settings.init() throw, which aborts the rest of index.js init() before it
-// renders anything at all — so a broken id does not degrade Settings, it
-// leaves the whole popup blank. These tests assert the controls are there
-// AND that they work, because "the view is visible" would still pass
-// against a screen whose handlers were never wired.
-
-// The four Token Spam Protection checkboxes, in markup order, with the
-// src/shared/state.js key each one is bound to. All four default true.
-const SPAM_FILTER_CHECKBOXES = [
-    { id: "settings-hide-spoofed-symbols", key: "hideSpoofedSymbols" },
-    { id: "settings-hide-low-holders", key: "hideLowHolderTokens" },
-    { id: "settings-hide-fraud-contracts", key: "hideFraudContracts" },
-    { id: "settings-hide-dust", key: "hideDustTransactions" },
-];
-
-// The one toggled through a reopen. Chosen because nothing later in this
-// suite depends on it: the other three filter token and transaction lists
-// that the ConfirmTx and dApp sections go on to drive.
-const TOGGLED_FILTER = "settings-hide-dust";
-
-// Everything the Settings assertions below must observe, recorded as each
-// group of them completes. The final test demands the exact set.
-//
-// The point is that a green run cannot mean the assertions were skipped.
-// Navigation that silently fails already fails a test — visible() throws
-// on a timeout — but an early return, a deleted test, or a body that
-// stopped being reached would otherwise shrink this section quietly
-// instead of reddening the run.
-const SETTINGS_COVERAGE = [
-    "about-well",
-    "spam-checkbox-defaults",
-    "theme-select",
-    "network-select",
-    "selector-round-trip",
-    "selector-restore",
-    "toggle-off-survives-reopen",
-    "toggle-on-survives-reopen",
-    "wallet-list",
-];
-
-// A control read as the DOM has it, not as a selector claims: tag name and
-// type distinguish a real <input type="checkbox"> from a <div> that merely
-// carries the id, and `checked` is the live property rather than the
-// attribute, so it reflects what init() assigned.
-function controlState(page, id) {
-    return page.evaluate((elementId) => {
-        const el = document.getElementById(elementId);
-        if (!el) return null;
-        return {
-            tag: el.tagName.toLowerCase(),
-            type: el.type || "",
-            checked: el.checked,
-            value: el.value,
-            options: Array.from(el.options || []).map((o) => o.value),
-        };
-    }, id);
-}
-
-async function checkboxStates(page) {
-    const out = {};
-    for (const { id } of SPAM_FILTER_CHECKBOXES) {
-        out[id] = await controlState(page, id);
-    }
-    return out;
-}
-
-function assertSpamCheckbox(st, id, expected, where) {
-    assert(st !== null, "no element with id " + id + " on Settings " + where);
-    assert(
-        st.tag === "input" && st.type === "checkbox",
-        id + " is a <" + st.tag + " type=" + st.type + ">, not a checkbox",
-    );
-    assert(
-        st.checked === expected,
-        id +
-            " reads " +
-            st.checked +
-            " " +
-            where +
-            ", expected " +
-            expected +
-            " — the checkbox is on screen but not carrying the persisted value",
-    );
-}
-
-test("Settings renders with the whole screen populated (#229)", async (env) => {
-    await visible(env.page, "#view-main");
-    await openSettings(env.page);
-
-    // show() writes the About well near its end — only the debug well and
-    // the debug-mode checkbox follow it — and showView() is the last thing
-    // of all, so an id show() cannot find aborts before Settings is ever
-    // displayed. Reading these values back proves show() ran through to
-    // there, not just far enough to unhide the section. They are filled
-    // from build-time constants that always have a value, so empty means
-    // the write did not happen.
-    const about = await env.page.evaluate(() => {
-        const out = {};
-        for (const id of [
-            "about-license",
-            "about-author",
-            "about-version",
-            "about-release-date",
-            "about-commit-link",
-        ]) {
-            const el = document.getElementById(id);
-            out[id] = el === null ? null : el.textContent.trim();
-        }
-        return out;
-    });
-    for (const [id, text] of Object.entries(about)) {
-        assert(
-            text !== null && text.length > 0,
-            "the About well left #" +
-                id +
-                " unwritten: " +
-                JSON.stringify(about),
-        );
-    }
-    env.settingsCoverage.add("about-well");
-
-    // The wallet list is rendered by settings.js rather than authored in
-    // index.html, so an empty container means renderWalletListSettings()
-    // did not run even though the screen came up.
-    const wallets = await env.page
-        .locator("#settings-wallet-list .settings-wallet-name")
-        .count();
-    assert(
-        wallets >= 2,
-        "Settings lists " +
-            wallets +
-            " wallets; the suite created two by this point",
-    );
-    env.settingsCoverage.add("wallet-list");
-});
-
-test("the four Token Spam Protection checkboxes render, defaulted on (#229)", async (env) => {
-    await openSettings(env.page);
-    const states = await checkboxStates(env.page);
-    for (const { id } of SPAM_FILTER_CHECKBOXES) {
-        assertSpamCheckbox(states[id], id, true, "on first render");
-    }
-    env.settingsCoverage.add("spam-checkbox-defaults");
-});
-
-test("the theme and network selectors render their real choices (#229)", async (env) => {
-    await openSettings(env.page);
-
-    const theme = await controlState(env.page, "settings-theme");
-    assert(theme !== null, "no #settings-theme element on Settings");
-    assert(
-        theme.tag === "select",
-        "#settings-theme is a <" + theme.tag + ">, not a <select>",
-    );
-    assert(
-        theme.options.join(",") === "system,light,dark",
-        "the theme selector offers " + JSON.stringify(theme.options),
-    );
-    env.settingsCoverage.add("theme-select");
-
-    const network = await controlState(env.page, "settings-network");
-    assert(network !== null, "no #settings-network element on Settings");
-    assert(
-        network.tag === "select",
-        "#settings-network is a <" + network.tag + ">, not a <select>",
-    );
-    const wantNetworks = Object.keys(NETWORKS).sort().join(",");
-    assert(
-        network.options.slice().sort().join(",") === wantNetworks,
-        "the network selector offers " +
-            JSON.stringify(network.options) +
-            ", expected the networks in src/shared/networks.js: " +
-            wantNetworks,
-    );
-    env.settingsCoverage.add("network-select");
-});
-
-// Reads both selectors in one page task, so a round trip cannot observe
-// them at two different moments.
-async function selectorValues(page) {
-    const theme = await controlState(page, "settings-theme");
-    const network = await controlState(page, "settings-network");
-    assert(theme !== null, "no #settings-theme element on Settings");
-    assert(network !== null, "no #settings-network element on Settings");
-    return { theme: theme.value, network: network.value };
-}
-
-function assertSelectors(got, wantTheme, wantNetwork, where) {
-    assert(
-        got.theme === wantTheme,
-        "the theme selector shows " +
-            JSON.stringify(got.theme) +
-            " " +
-            where +
-            ", expected " +
-            JSON.stringify(wantTheme),
-    );
-    assert(
-        got.network === wantNetwork,
-        "the network selector shows " +
-            JSON.stringify(got.network) +
-            " " +
-            where +
-            ", expected " +
-            JSON.stringify(wantNetwork),
-    );
-}
-
-// The two values the selectors are driven to. NEITHER is the first
-// <option> of its <select> (`system` and `mainnet` are), and that is the
-// entire point: the first option is what the DOM reports with no
-// JavaScript involved at all, so asserting it would pass just as happily
-// against a Settings screen that never assigned anything. Only a value
-// that went out through the change handler and saveState(), and came back
-// through loadState() and the assignment show()/init() makes, can be read
-// here.
-const NONDEFAULT_THEME = "dark";
-const NONDEFAULT_NETWORK = "sepolia";
-
-test("the theme and network selectors carry a non-default persisted value (#229)", async (env) => {
-    await openSettings(env.page);
-
-    // selectOption() fires "change", which is what the handlers bind.
-    await env.page.selectOption("#settings-theme", NONDEFAULT_THEME);
-    await env.page.selectOption("#settings-network", NONDEFAULT_NETWORK);
-
-    await reopenPopup(env, "#view-settings");
-
-    assertSelectors(
-        await selectorValues(env.page),
-        NONDEFAULT_THEME,
-        NONDEFAULT_NETWORK,
-        "after reopening the popup",
-    );
-    env.settingsCoverage.add("selector-round-trip");
-
-    // Restore, the same way round, and assert the restore actually took
-    // rather than trusting it: the later sections inherit this fixture,
-    // and a selector stuck on `dark`/`sepolia` would otherwise be
-    // indistinguishable here from one that persists correctly. Switching
-    // the network back also returns state.rpcUrl and state.blockscoutUrl
-    // to the mainnet defaults that onChainSwitch() overwrote, which are
-    // the values src/shared/state.js starts with.
-    await env.page.selectOption("#settings-theme", "system");
-    await env.page.selectOption("#settings-network", "mainnet");
-
-    await reopenPopup(env, "#view-settings");
-
-    assertSelectors(
-        await selectorValues(env.page),
-        "system",
-        "mainnet",
-        "after restoring and reopening the popup",
-    );
-    env.settingsCoverage.add("selector-restore");
-});
-
-// The functional half. A checkbox that renders but is not wired looks
-// identical on screen; only a value that survives being written to storage
-// and read back by a fresh page load tells the two apart. That round trip
-// runs through the change handler, saveState(), loadState() and the
-// assignment init() makes — every part of the wiring at once.
-test("a spam filter toggled in Settings survives a popup reopen (#229)", async (env) => {
-    await openSettings(env.page);
-    await env.page.click("#" + TOGGLED_FILTER);
-
-    const immediately = await controlState(env.page, TOGGLED_FILTER);
-    assert(
-        immediately.checked === false,
-        "clicking #" + TOGGLED_FILTER + " did not clear it",
-    );
-
-    await reopenPopup(env, "#view-settings");
-
-    const after = await checkboxStates(env.page);
-    for (const { id } of SPAM_FILTER_CHECKBOXES) {
-        assertSpamCheckbox(
-            after[id],
-            id,
-            id !== TOGGLED_FILTER,
-            "after reopening the popup",
-        );
-    }
-    env.settingsCoverage.add("toggle-off-survives-reopen");
-});
-
-test("turning the same filter back on survives a reopen too (#229)", async (env) => {
-    await openSettings(env.page);
-    await env.page.click("#" + TOGGLED_FILTER);
-
-    await reopenPopup(env, "#view-settings");
-
-    // Restores the fixture the later sections inherit, and rules out a
-    // checkbox that persists "off" only because it is stuck there.
-    const after = await checkboxStates(env.page);
-    for (const { id } of SPAM_FILTER_CHECKBOXES) {
-        assertSpamCheckbox(after[id], id, true, "after toggling back on");
-    }
-    env.settingsCoverage.add("toggle-on-survives-reopen");
-
-    await env.page.click("#btn-settings-back");
-    await visible(env.page, "#view-main");
-});
-
-test("the Settings assertions above all ran (#229)", async (env) => {
-    const seen = [...env.settingsCoverage].sort();
-    const want = SETTINGS_COVERAGE.slice().sort();
-    assert(
-        seen.join(",") === want.join(","),
-        "the Settings section covered " +
-            JSON.stringify(seen) +
-            " but must cover " +
-            JSON.stringify(want) +
-            " — a green run here would otherwise mean only that fewer " +
-            "assertions ran, not that they passed",
-    );
-});
-
 // ------------------------------------------------ dust threshold (#233)
 
 // The popup size README documents the UI as designed for. Pages in this
@@ -3069,11 +2746,6 @@ async function main() {
         // The recovery phrase of the wallet created in test 2, so later
         // tests can assert on the real secret rather than its shape.
         phrase: null,
-        // What the Settings section (#229) actually observed. A guard test
-        // at the end of that section demands the full set, so a skipped or
-        // silently shortened assertion reddens the run instead of shrinking
-        // it.
-        settingsCoverage: new Set(),
         // Confirmation-screen heights, measured in the pending state and
         // compared against every later state of the same screen.
         ethPendingHeight: null,

tests/popupElementIds.test.js

@@ -1,182 +0,0 @@
-// Every element id the popup views look up must exist in the markup they
-// look it up in.
-//
-// The failure this catches: `$("settings-hide-dsut")` is valid JavaScript
-// referring to a defined function, so neither jest (node environment, no
-// DOM) nor a linter has anything to object to. At runtime `$()` returns
-// null and the next property access throws, which in `init()` aborts the
-// rest of that view's wiring and takes the whole screen down. Settings is
-// the densest concentration of these lookups in the codebase.
-//
-// This is the cheap general half of the guard: it runs in `make check`
-// with no browser and covers every id in every view, not the ones some
-// test happens to click. The expensive specific half is the Settings
-// section of the end-to-end suite (tests/e2e/run.js), which proves the
-// screen actually comes up and its controls work.
-//
-// Scope and limits, stated rather than implied:
-//   - Only literal string arguments are resolvable statically. A call
-//     like `$(containerId)` is invisible here; those are covered by the
-//     e2e run instead.
-//   - `document.getElementById()` is checked too, minus the ids listed in
-//     RUNTIME_CREATED_IDS, which name nodes the code creates itself and
-//     which are legitimately absent from the static markup.
-
-"use strict";
-
-const fs = require("fs");
-const path = require("path");
-
-const POPUP_DIR = path.join(__dirname, "..", "src", "popup");
-const POPUP_HTML_PATH = path.join(POPUP_DIR, "index.html");
-
-// Nodes built at runtime rather than authored in index.html. Each one must
-// be created unconditionally by the code before it is ever looked up.
-const RUNTIME_CREATED_IDS = new Set([
-    // Created by updateDebugBanner() in src/popup/views/helpers.js.
-    "debug-banner",
-]);
-
-// Every id lookup the popup performs with a literal argument, as
-// {id, file, line, source} records.
-//
-// showView("x") is included because it resolves to the element id
-// "view-x": a view name with no matching section is the same defect one
-// indirection further out.
-const PATTERNS = [
-    { re: /\$\(\s*"([^"\n]+)"\s*\)/g, id: (m) => m[1], source: "$()" },
-    {
-        re: /document\.getElementById\(\s*"([^"\n]+)"\s*\)/g,
-        id: (m) => m[1],
-        source: "getElementById()",
-    },
-    {
-        re: /\b(?:showError|hideError)\(\s*"([^"\n]+)"/g,
-        id: (m) => m[1],
-        source: "showError()/hideError()",
-    },
-    {
-        re: /\bshowView\(\s*"([^"\n]+)"\s*\)/g,
-        id: (m) => "view-" + m[1],
-        source: "showView()",
-    },
-];
-
-function jsFilesUnder(dir) {
-    const out = [];
-    for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
-        const full = path.join(dir, entry.name);
-        if (entry.isDirectory()) {
-            out.push(...jsFilesUnder(full));
-        } else if (entry.name.endsWith(".js")) {
-            out.push(full);
-        }
-    }
-    return out.sort();
-}
-
-function lineOf(text, index) {
-    return text.slice(0, index).split("\n").length;
-}
-
-function collectReferences() {
-    const refs = [];
-    for (const file of jsFilesUnder(POPUP_DIR)) {
-        const text = fs.readFileSync(file, "utf8");
-        const rel = path.relative(path.join(__dirname, ".."), file);
-        for (const { re, id, source } of PATTERNS) {
-            re.lastIndex = 0;
-            let m;
-            while ((m = re.exec(text)) !== null) {
-                refs.push({
-                    id: id(m),
-                    file: rel,
-                    line: lineOf(text, m.index),
-                    source,
-                });
-            }
-        }
-    }
-    return refs;
-}
-
-function collectHtmlIds(html) {
-    const ids = [];
-    const re = /\bid="([^"]+)"/g;
-    let m;
-    while ((m = re.exec(html)) !== null) ids.push(m[1]);
-    return ids;
-}
-
-const HTML = fs.readFileSync(POPUP_HTML_PATH, "utf8");
-const HTML_IDS = collectHtmlIds(HTML);
-const HTML_ID_SET = new Set(HTML_IDS);
-const REFERENCES = collectReferences();
-
-describe("every element id the popup looks up exists in its markup", () => {
-    // A guard that found nothing to check would pass forever. If a
-    // refactor renames the directory, changes the helper, or moves the
-    // markup, this fails instead of quietly covering zero call sites.
-    // The floors are far below the counts measured when this was written
-    // (434 lookups across 20 of the 24 files under src/popup/, against 274
-    // ids in the markup), so ordinary churn does not trip them.
-    test("the scan actually found the code and the markup", () => {
-        const files = new Set(REFERENCES.map((r) => r.file));
-        expect(files.size).toBeGreaterThanOrEqual(15);
-        expect(REFERENCES.length).toBeGreaterThanOrEqual(300);
-        expect(HTML_IDS.length).toBeGreaterThanOrEqual(200);
-
-        // The densest screen, named explicitly: a scan that stopped
-        // covering src/popup/views/settings.js is the exact regression
-        // this file was written for.
-        expect(
-            files.has(path.join("src", "popup", "views", "settings.js")),
-        ).toBe(true);
-        expect(
-            REFERENCES.some((r) => r.id === "settings-hide-spoofed-symbols"),
-        ).toBe(true);
-        expect(REFERENCES.some((r) => r.id === "view-settings")).toBe(true);
-    });
-
-    test("no lookup names an id that src/popup/index.html does not define", () => {
-        const missing = REFERENCES.filter(
-            (r) => !HTML_ID_SET.has(r.id) && !RUNTIME_CREATED_IDS.has(r.id),
-        ).map(
-            (r) =>
-                r.file +
-                ":" +
-                r.line +
-                " " +
-                r.source +
-                ' looks up id "' +
-                r.id +
-                '", which is not in src/popup/index.html',
-        );
-
-        expect(missing).toEqual([]);
-    });
-
-    test("every id excused as runtime-created is still looked up somewhere", () => {
-        // Otherwise the exception list becomes a place stale names
-        // accumulate, and the next real miss can be waved through by
-        // adding one more.
-        for (const id of RUNTIME_CREATED_IDS) {
-            expect(REFERENCES.some((r) => r.id === id)).toBe(true);
-            expect(HTML_ID_SET.has(id)).toBe(false);
-        }
-    });
-
-    test("index.html defines no id twice", () => {
-        // getElementById returns the first match, so a duplicate id means
-        // one of the two elements can never be reached by the code that
-        // thinks it owns it.
-        const seen = new Set();
-        const duplicated = [];
-        for (const id of HTML_IDS) {
-            if (seen.has(id)) duplicated.push(id);
-            seen.add(id);
-        }
-
-        expect(duplicated).toEqual([]);
-    });
-});