diff --git a/TODO.md b/TODO.md index 4311a3c..81297cb 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,18 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-06: Reloading or closing the popup no longer logs a request it cancels + as a failure on the address and token screens, in the address scan after a + wallet is created, or in the endpoint checks in Settings + ([#475](https://git.eeqj.de/sneak/AutistMask/issues/475)). Each checks the + signal the popup aborts on `pagehide` + ([#218](https://git.eeqj.de/sneak/AutistMask/issues/218)) before reporting a + failure; a real failure is still logged. The e2e suite reloads the popup on + the address screen with Blockscout held, and during the address scan with that + scan held; jest tests cover the transaction lists, the scan and the Settings + checks. The transaction detail and confirmation screens are not covered: they + do not keep the popup context that carries the signal. + - 2026-10-06: Reloading or closing the popup mid-refresh no longer logs the requests that cancels as failures ([#218](https://git.eeqj.de/sneak/AutistMask/issues/218)). Chrome cancels a diff --git a/src/popup/views/addWallet.js b/src/popup/views/addWallet.js index c9b7087..97f7f9a 100644 --- a/src/popup/views/addWallet.js +++ b/src/popup/views/addWallet.js @@ -190,7 +190,12 @@ async function importMnemonic(ctx) { // Scan for used HD addresses beyond index 0. showFlash("Scanning for addresses...", 30000); - const scan = await scanForAddresses(xpub, state.rpcUrl, state.networkId); + const scan = await scanForAddresses( + xpub, + state.rpcUrl, + state.networkId, + ctx.pageClosed, + ); if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, @@ -300,7 +305,12 @@ async function importXprvKey(ctx) { // Scan for used HD addresses beyond index 0. showFlash("Scanning for addresses...", 30000); - const scan = await scanForAddresses(xpub, state.rpcUrl, state.networkId); + const scan = await scanForAddresses( + xpub, + state.rpcUrl, + state.networkId, + ctx.pageClosed, + ); if (scan.addresses.length > 1) { wallet.addresses = scan.addresses.map((a) => ({ address: a.address, diff --git a/src/popup/views/addressDetail.js b/src/popup/views/addressDetail.js index f100536..7ecc27a 100644 --- a/src/popup/views/addressDetail.js +++ b/src/popup/views/addressDetail.js @@ -143,6 +143,9 @@ async function loadTransactions(address) { renderTransactions(txs); } catch (e) { + // Cancelled by the popup closing, not failed: see pageClosed in + // src/popup/index.js. + if (ctx.pageClosed.aborted) return; log.errorf("loadTransactions failed:", e.message); $("tx-list").innerHTML = '
Failed to load transactions.
'; diff --git a/src/popup/views/addressToken.js b/src/popup/views/addressToken.js index a544789..9fb8db9 100644 --- a/src/popup/views/addressToken.js +++ b/src/popup/views/addressToken.js @@ -227,6 +227,9 @@ async function loadTransactions(address, tokenId) { renderTransactions(txs); } catch (e) { + // Cancelled by the popup closing, not failed: see pageClosed in + // src/popup/index.js. + if (ctx.pageClosed.aborted) return; log.errorf("loadTransactions failed:", e.message); $("address-token-tx-list").innerHTML = '
Failed to load transactions.
'; diff --git a/src/popup/views/settings.js b/src/popup/views/settings.js index cd79548..4661fd4 100644 --- a/src/popup/views/settings.js +++ b/src/popup/views/settings.js @@ -273,6 +273,9 @@ function init(ctx) { return; } } catch { + // Cancelled by the popup closing, not failed: see pageClosed in + // src/popup/index.js. + if (ctx.pageClosed.aborted) return; // Not the error's message: fetch puts the whole URL, password and // key included, in the message of the error it throws for a URL // with a user name and password or one it cannot parse. @@ -299,6 +302,7 @@ function init(ctx) { return; } } catch { + if (ctx.pageClosed.aborted) return; // Not the error's message, as for the RPC check above. log.errorf("Blockscout validation failed:", urlOrigin(url)); showFlash("Could not reach endpoint."); diff --git a/src/shared/balances.js b/src/shared/balances.js index eabbed2..10c35e1 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -339,7 +339,8 @@ async function lookupTokenInfo(contractAddress, rpcUrl, networkId) { // Checks gapLimit addresses in parallel per batch. Stops when an entire // batch has no used addresses (i.e. gapLimit consecutive empty addresses). // Returns { addresses: [{ address, index }], nextIndex }. -async function scanForAddresses(xpub, rpcUrl, networkId, gapLimit = 5) { +// `signal` is as for fetchTokenBalances(). +async function scanForAddresses(xpub, rpcUrl, networkId, signal, gapLimit = 5) { log.debugf("scanForAddresses start, gapLimit:", gapLimit); const provider = getProvider(rpcUrl, networkId); const used = []; @@ -362,11 +363,13 @@ async function scanForAddresses(xpub, rpcUrl, networkId, gapLimit = 5) { ]); return { addr, index, isUsed: balance > 0n || txCount > 0 }; } catch (e) { - log.errorf( - "scanForAddresses check failed", - addr, - e.shortMessage || e.message, - ); + if (!signal?.aborted) { + log.errorf( + "scanForAddresses check failed", + addr, + e.shortMessage || e.message, + ); + } return { addr, index, isUsed: false }; } }), diff --git a/tests/balanceRefreshCancelled.test.js b/tests/balanceRefreshCancelled.test.js index c9e56d4..5852484 100644 --- a/tests/balanceRefreshCancelled.test.js +++ b/tests/balanceRefreshCancelled.test.js @@ -1,13 +1,19 @@ -// The balance refresh does not report a request the popup's own closing -// cancelled, and still reports one that failed while the popup was open -// (https://git.eeqj.de/sneak/AutistMask/issues/218). +// The balance refresh, and the address scan after a wallet is created, do not +// report a request the popup's own closing cancelled, and still report one +// that failed while the popup was open +// (https://git.eeqj.de/sneak/AutistMask/issues/218, +// https://git.eeqj.de/sneak/AutistMask/issues/475). // // In the popup a cancelled fetch() fails with the same "Failed to fetch" as a // server that cannot be reached, so every request here fails that way, and // only the signal the popup aborts on pagehide tells the two cases apart. const { FetchRequest } = require("ethers"); -const { refreshBalances } = require("../src/shared/balances"); +const { refreshBalances, scanForAddresses } = require("../src/shared/balances"); +const { + generateMnemonic, + hdWalletFromMnemonic, +} = require("../src/shared/wallet"); const RPC_URL = "https://rpc.example.invalid"; const EXPLORER_URL = "https://explorer.example.invalid/api/v2"; @@ -24,8 +30,9 @@ beforeEach(() => { const failedToFetch = async () => { throw new TypeError("Failed to fetch"); }; - // The RPC calls (ETH balance, ENS name) and the explorer request (token - // balances) all fail the same way. + // The RPC calls (ETH balance, ENS name, and the scan's balance and + // transaction count) and the explorer request (token balances) all fail + // the same way. FetchRequest.registerGetUrl(failedToFetch); globalThis.fetch = jest.fn(failedToFetch); }); @@ -65,3 +72,24 @@ test("a failure once the popup has closed is not", async () => { await refresh(pageClosed.signal); expect(logged).toEqual([]); }); + +function scan(signal) { + // What the scan found is logged at info level, which is not under test. + jest.spyOn(console, "log").mockImplementation(() => {}); + const { xpub } = hdWalletFromMnemonic(generateMnemonic()); + return scanForAddresses(xpub, RPC_URL, "mainnet", signal); +} + +test("a scan failure while the popup is open is reported", async () => { + await scan(new AbortController().signal); + expect( + logged.some((line) => line.includes("scanForAddresses check failed")), + ).toBe(true); +}); + +test("a scan failure once the popup has closed is not", async () => { + const pageClosed = new AbortController(); + pageClosed.abort(); + await scan(pageClosed.signal); + expect(logged).toEqual([]); +}); diff --git a/tests/e2e/network.js b/tests/e2e/network.js index 86a5993..efa0fa6 100644 --- a/tests/e2e/network.js +++ b/tests/e2e/network.js @@ -420,12 +420,12 @@ const HOLD_MAX_MS = 30000; // Hold a reply open for as long as the test asks: until opts[name] is false. // -// The switch (holdGasEstimate or holdBlockscout) is read here rather than -// captured, so a test flips it on the same options object the route was -// registered with — the same pattern as seedTokenTransfer. This is the only way -// to observe a screen while its request is genuinely in flight; sampling the -// screen and hoping to win a race against the network would assert nothing on -// a slow machine. +// The switch (holdGasEstimate, holdTransactionCount or holdBlockscout) is read +// here rather than captured, so a test flips it on the same options object the +// route was registered with — the same pattern as seedTokenTransfer. This is +// the only way to observe a screen while its request is genuinely in flight; +// sampling the screen and hoping to win a race against the network would +// assert nothing on a slow machine. // // It never gives up quietly. A hold that outlives the bound is reported like // any other harness fault, because a "pending" state that stopped being @@ -560,6 +560,9 @@ async function handleRpc(route, postData, opts, report) { if (batch.some((req) => req.method === "eth_estimateGas")) { await awaitRelease(opts, "holdGasEstimate", report); } + if (batch.some((req) => req.method === "eth_getTransactionCount")) { + await awaitRelease(opts, "holdTransactionCount", report); + } const replies = batch.map((req) => rpcReply(req, opts, report)); return jsonResponse(route, Array.isArray(payload) ? replies : replies[0]); @@ -614,6 +617,8 @@ function traceEnabled(raw) { * node-side refusal. * @param {boolean} [opts.holdGasEstimate] hold every batch containing an * eth_estimateGas until this is cleared again. + * @param {boolean} [opts.holdTransactionCount] hold every batch containing an + * eth_getTransactionCount until this is cleared again. * @param {boolean} [opts.holdBlockscout] hold every Blockscout request until * this is cleared again. * @param {boolean} [opts.failTransactionList] fail every request for an diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 797c52a..367bd90 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -249,8 +249,23 @@ test("the exact confirmation phrase erases the record and reloads into Welcome ( } }); -test("wallet creation through the UI reaches the main view", async (env) => { - env.phrase = await createWallet(env.page); +// The address scan that follows creating the wallet is held, so the popup +// reloads while it is in flight. As for the reloads mid-refresh further down, +// the assertion for that is the harness's own: a console.error from the page +// being reloaded fails this test (#475). +test("wallet creation through the UI reaches the main view, and reloading mid-scan reports no failure (#475)", async (env) => { + env.routeOpts.holdTransactionCount = true; + try { + const scanning = env.page.waitForRequest((r) => + (r.postData() || "").includes("eth_getTransactionCount"), + ); + env.phrase = await createWallet(env.page); + await scanning; + await env.page.reload(); + } finally { + env.routeOpts.holdTransactionCount = false; + } + await visible(env.page, "#view-main"); assert( env.phrase.split(/\s+/).length >= 12, "wallet creation did not yield a recovery phrase", @@ -700,9 +715,8 @@ test("the token contract row links to the explorer's token page (#151)", async ( // // Reloading or closing the popup makes Chrome cancel the requests it still has // open, and in the page a cancelled fetch() fails with the same "Failed to -// fetch" as a server that cannot be reached. Here rather than straight after -// wallet creation because a reload also cancels the address scan that follows -// it, which still logs (#475). +// fetch" as a server that cannot be reached. The reload during the address +// scan that follows creating a wallet is in the wallet creation test (#475). // Blockscout is held, so the home screen's transaction list and token balances // cannot have been answered when the popup reloads. The assertion is the @@ -752,6 +766,30 @@ test("a transaction list that cannot be fetched is still reported (#218)", async } }); +// The address screen's transaction list, held the same way. The popup reopens +// on the address screen, which asks for its list as it is drawn, so once the +// screen is up its request is in flight (#475). +test("reloading the popup on the address screen reports no failure (#475)", async (env) => { + await openAddressDetail(env.page); + await waitForPersisted( + env.page, + "currentView", + "address", + "before reloading the popup", + ); + env.routeOpts.holdBlockscout = true; + try { + await env.page.reload(); + await visible(env.page, "#view-address"); + await env.page.reload(); + } finally { + env.routeOpts.holdBlockscout = false; + } + await visible(env.page, "#view-address"); + // Home again, where the tests below expect to start. + await goHome(env.page); +}); + // -------------------------------------------- recovery phrase (#161) // The gear toggles, so pressing it while Settings is already up leaves it. @@ -2163,9 +2201,7 @@ function quantity(wei) { // Wait on the main view until a changed balance fixture has been picked up. // -// Not a reload: the popup re-refreshes on a 10-second timer by itself, and a -// reload on the address screen still logs the transaction list it cancels -// (#475). +// Not a reload: the popup re-refreshes on a 10-second timer by itself. // // It also deliberately settles on MAIN rather than on the address screen. // The address screen builds the send screen's token dropdown once, from the @@ -4525,6 +4561,9 @@ async function main() { ethBalanceWei: null, failGasEstimate: false, holdGasEstimate: false, + // Whether a JSON-RPC batch asking for a transaction count is held + // unanswered: the address scan after creating a wallet (#475). + holdTransactionCount: false, // Whether Blockscout requests are held unanswered, and whether an // address's transaction list fails as a network error (#218). holdBlockscout: false, diff --git a/tests/settingsEndpointCheck.test.js b/tests/settingsEndpointCheck.test.js index 988097f..8f73ba7 100644 --- a/tests/settingsEndpointCheck.test.js +++ b/tests/settingsEndpointCheck.test.js @@ -50,7 +50,8 @@ function element(id) { return (elements[id] ||= fakeElement()); } -function loadSettingsView() { +// `pageClosed` stands in for the popup's: aborting it is the popup closing. +function loadSettingsView(pageClosed = new AbortController()) { elements = {}; flashes = []; @@ -74,7 +75,9 @@ function loadSettingsView() { state.blockscoutUrl = SAVED_BLOCKSCOUT; require("../src/shared/log").setRuntimeDebug(true); - require("../src/popup/views/settings").init({}); + require("../src/popup/views/settings").init({ + pageClosed: pageClosed.signal, + }); } async function save(fieldId, buttonId, typed) { @@ -146,3 +149,21 @@ test("the Blockscout check of a URL with a user name and password", async () => const line = expectFailedWithoutSecrets("Blockscout validation failed"); expect(line).toContain("https://explorer.example.invalid"); }); + +// In the popup a check its own closing cancelled fails as these do, and is +// not reported (https://git.eeqj.de/sneak/AutistMask/issues/475). Nothing is +// saved either. +test("a check that fails once the popup has closed is not reported", async () => { + const pageClosed = new AbortController(); + pageClosed.abort(); + loadSettingsView(pageClosed); + await save("settings-rpc", "btn-save-rpc", RPC_UNPARSEABLE); + await save( + "settings-blockscout", + "btn-save-blockscout", + BLOCKSCOUT_WITH_PASSWORD, + ); + expect(console.error).not.toHaveBeenCalled(); + expect(state.rpcUrl).toBe(SAVED_RPC); + expect(state.blockscoutUrl).toBe(SAVED_BLOCKSCOUT); +}); diff --git a/tests/transactionListCancelled.test.js b/tests/transactionListCancelled.test.js new file mode 100644 index 0000000..0e0d6a0 --- /dev/null +++ b/tests/transactionListCancelled.test.js @@ -0,0 +1,127 @@ +// The address and token screens do not report a transaction list the popup's +// own closing cancelled, and still report one that failed while the popup was +// open (https://git.eeqj.de/sneak/AutistMask/issues/475). +// +// In the popup a cancelled fetch() fails with the same "Failed to fetch" as a +// server that cannot be reached, so the explorer request here always fails +// that way, and only the signal the popup aborts on pagehide tells the two +// cases apart. Driven against a minimal DOM stub in the shape +// tests/timestampDisplay.test.js uses. + +jest.mock("../src/shared/transactions", () => ({ + ...jest.requireActual("../src/shared/transactions"), + fetchRecentTransactions: async () => { + throw new TypeError("Failed to fetch"); + }, +})); + +globalThis.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, +}; + +const { state } = require("../src/shared/state"); +const addressDetail = require("../src/popup/views/addressDetail"); +const addressToken = require("../src/popup/views/addressToken"); + +const ADDRESS = "0x1111111111111111111111111111111111111111"; + +function makeElement(id) { + const el = { + id, + textContent: "", + value: "", + innerHTML: "", + style: {}, + dataset: {}, + classList: { + add: () => {}, + remove: () => {}, + contains: () => false, + toggle: () => false, + }, + addEventListener: () => {}, + querySelectorAll: () => [], + appendChild: () => {}, + }; + // Views reach for .parentElement to hide whole sections. + Object.defineProperty(el, "parentElement", { + get: () => node(id + "-parent"), + }); + return el; +} + +function makeDocument() { + const els = new Map(); + return { + getElementById(id) { + // The debug banner is created on demand by helpers.js; absent + // is the state a non-debug, non-testnet popup is in. + if (id === "debug-banner") return null; + if (!els.has(id)) els.set(id, makeElement(id)); + return els.get(id); + }, + createElement: () => makeElement("created"), + addEventListener: () => {}, + body: { prepend: () => {} }, + }; +} + +function node(id) { + return globalThis.document.getElementById(id); +} + +let logged; + +beforeEach(() => { + logged = []; + jest.spyOn(console, "error").mockImplementation((...args) => { + logged.push(args.map(String).join(" ")); + }); + globalThis.document = makeDocument(); + globalThis.window = { location: { search: "" } }; + state.wallets = [ + { + name: "Main", + type: "key", + addresses: [{ address: ADDRESS, balance: "0.0000" }], + }, + ]; + state.trackedTokens = []; + state.viewStack = []; + state.selectedWallet = 0; + state.selectedAddress = 0; + state.selectedToken = "ETH"; +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +describe.each([ + ["the address screen", addressDetail, "tx-list"], + ["the token screen", addressToken, "address-token-tx-list"], +])("the transaction list on %s", (_name, view, listId) => { + // Open the screen and wait for its transaction list to fail. + async function open(pageClosed) { + view.init({ pageClosed }); + view.show(); + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + test("a failure while the popup is open is reported", async () => { + await open(new AbortController().signal); + expect(logged).toEqual([ + "[AutistMask] loadTransactions failed: Failed to fetch", + ]); + expect(node(listId).innerHTML).toContain( + "Failed to load transactions.", + ); + }); + + test("a failure once the popup has closed is not", async () => { + const pageClosed = new AbortController(); + pageClosed.abort(); + await open(pageClosed.signal); + expect(logged).toEqual([]); + }); +});